-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Page] Improve the accessibility of the rollup actions #4080
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
Conversation
|
👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. |
|
🟢 This pull request modifies 5 files and might impact 3 other files. Details:All files potentially affected (total: 3)📄
|
2c6e842 to
d2611d6
Compare
12c8ef1 to
f2f4941
Compare
size-limit report
|
794a70c to
43ce39f
Compare
kyledurand
left a comment
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.
I'm wondering if the tooltip is 100% necessary because it's adding more clutter to the already cluttered page header the a11y label on button should be enough for screenreaders right? @sarahill do you have thoughts on this?
|
@kyledurand thanks for taking a look! Adding the tooltip was to help voice-command users as @emma-boardman described in this comment. |
emma-boardman
left a comment
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.
Apologies for the delayed review!
- Label overrides
I can't remember if this was discussed previously (apologies if it was), but is there a way to encourage consistency with the custom labels, and thus across the admin? For example, View actions for ${my-custom-label}?
- Tooltip
Agree this could look cluttered depending on the page in question 🤔
Maybe next time there's a design language iteration we could do some Fable testing and see if there's a way to better serve voice-activation users within our mobile UI.
382ed73 to
e308d67
Compare
bb1251b to
79e3f3b
Compare
|
I've removed the tooltip and we can revisit that later, particularly because depending on hover is not a pattern we can use on mobile. I also updated the label to use the I'll re-request the translation once approved. |
|
➕ 1 to removing the tooltip for now. We're starting to look at things like this across the UI now so thanks for bringing this up. I will keep it in mind 👍 |
kyledurand
left a comment
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.
Looks great. Thanks @lsit !
997a41b to
d51e45d
Compare
d51e45d to
f40da47
Compare
|
🎉 Thanks for your contribution to Polaris React! |
WHY are these changes introduced?
Resolves #4072
WHAT is this pull request doing?
RollupActionsbutton to be "View actions" to better match our content guidelinesRollupActionsbutton within thePageheader to be related to the title "View actions for {title}". This will provide consistency across all pages.Media
How to 🎩
Page with all header elementsstory on small screen🎩 checklist
README.mdwith documentation changes