-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add meter example for issue 1105 #1190
Conversation
Also remove sectionhead as it is an abstract role, and include an early draft of accessible name calculation.
- Use "explicit label" and "implicit label" instead of "name from author" and "name from content". - Expand on accessible name calculation.
Also remove sectionhead as it is an abstract role, and include an early draft of accessible name calculation.
- Use "explicit label" and "implicit label" instead of "name from author" and "name from content". - Expand on accessible name calculation.
Thank you for your changes @smhigley, this looks great! I think the SVG would be better if that means we can retain the fill. To me it feels wrong to only show the two shapes with borders. Feels like that takes away some clarity of what this is. What do you think? |
Looks good, @smhigley ! I agree with @ZoeBijl that the meter would be "more clearly a meter" if we could remove the inner border. (using SVG sounds fine - but curious why we wouldn't just change the border color in js at the same time as the fill color? Sorry if that's a CSS noob question! 😄 ) Also just wondering whether having the Pause button after the meter would feel more natural? |
@carmacleod putting the pause button after the meter makes sense to me! The reason I have a border on there is because not all colors between green and red pass 3:1 contrast (particularly in the yellow range). @mcking65 / @spectranaut independent of what we do with the fill and pause button position, the tests should be ready for review! I remember Matt saying something about wanting to get it done on Friday on the call :) |
Ah, of course. I misunderstood what @ZoeBijl said earlier, but I get it now. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@carmacleod I don't think you misread Zoë's comment, if you mean the one from before I added my changes. Using just |
@smhigley Actually, I meant this comment:
I misread the 2nd sentence as:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test review! Looks good to me.
Two minor things:
- we have a norm of writing a test for the role as well -- there is a helper function called assertAriaRole. Really the only thing it does is make sure the role is on the element type listed in the attribute table.
- There is an ongoing discussion of whether or not to use
t.plan()
when it's not strictly necessary. None of these tests needt.plan()
(there is no branching) but I'm personally in favor of adding it everywhere in order to prevent forgetting it. Feel free to contribute opinions to that discussion :)
Edit: We are going to move forward without t.plan. So you can mark tests are reviewed and completed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved for the visual and accessibility reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval for test review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial review and corrections complete.
I made some minor phrasing changes for consistency with other parts of the APG, fixed some link targets, and removed list markup where there was only 1 list item.
The pause button is not documented or tested. I created #1222 for this.
Seeing that all other reviews are complete, this example is ready to ship!
Resolves #1105
initial draft of meter example.
Review checklist
Preview | Diff