-
Notifications
You must be signed in to change notification settings - Fork 2k
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
My Home: Add aria labels and list markup to site setup checklist #53565
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~49 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
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.
Is this change worth the dev effort? Or perhaps worthy of a comment in the code, in the event of a larger piece of work or refactor , then refactoring to GB accordion could be integrated with another piece of work. |
Yes, because at the moment users who use assistive technology to navigate Calypso will struggle to work through the setup checklist. Is it specific to the work of the My Home pod? Probably not. But a11y (and security, and performance, and supporting mobile web) are just part of the status quo of development imo. |
Oh I think I see what you mean now, maybe we should have a
Yeah lets do this instead, I'm not sure TODO comments in code ever lead to much action 😅 |
From a visual/user POV this is looking good to me. Seems like a bonus win that the animation when opening a task with the accordion (in the mobile view) is much smoother looking |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/6014267 Thank you @p-jackson for including a screenshot in the description! This is really helpful for our translators. |
Ticket created #53617 |
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
aria-label
s for the icons in the checklistThe diff in GH makes it look scarier than it is. Ignoring the indentation change it wraps the checklist in a
<ul>
and replaces a<Fragment>
with a<li>
.This change is important because we want to be supporting users who use assistive technology anyway. But this came up because we were discussing whether to change the task labels when the task gets completed. Previously that would have been the only way for screen readers to tell the difference between a complete and incomplete task. Adding the aria labels in this PR means we can make the decision about task labels independently of a11y concerns.
In mobile view the checklist switches to an accordian view. We should really be using GB's according component for this since it has better support for assistive technology.
Testing instructions
This change has actually improved the placement of one of the borders in accordian view.
![Screenshot 2021-06-10 at 10 49 19 AM](https://user-images.githubusercontent.com/1500769/121440312-c1b7c880-c9db-11eb-978d-b8a7c73e2f3e.png)
Before
After
![Screenshot 2021-06-10 at 10 49 06 AM](https://user-images.githubusercontent.com/1500769/121440329-c8464000-c9db-11eb-889c-c8d38babbf73.png)
Related to the discussion in #52936, although not the main issue.