-
Notifications
You must be signed in to change notification settings - Fork 229
docs(tab): update tab docs and a11y #5586
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
base: main
Are you sure you want to change the base?
Conversation
|
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
@aramos-adobe before adding the |
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.
Some minor changes, but overall all looking good. I like the details you've added to the docs.
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.
Nice work on this! I think there's some documentation changes that need to be made to clear up the focused state section. To me, they conflicted with the accessibility portion at the bottom, but I left some comments to try to connect my thoughts.
packages/tabs/tab.md
Outdated
slot="icon" | ||
label="Checking your work" | ||
aria-label="Tab w/ checkmark" |
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'd have to check with Nikki again, but would we want the label
and aria-label
to be the same, that way screen reader users get the same experience & information?
When I tested this in Safari, VoiceOver also announces: "Tab w slash checkmark" instead of what this was probably going for "Tab with checkmark."
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.
Or maybe the aria-label
replaces label
on sp-tab
?
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.
Now that I think of it, it makes sense to have only the aria-label
exist for icon only situations and it should be on sp-tab
Description
Ensure the full API is documented, including each property, slot, method, event, CSS property, and tokens in the component and along with accessibility concerns and/or decisions, linking to accessibility team documentation where applicable for the tab component.
Related issue(s)
Author's checklist
Reviewer's checklist
patch
,minor
, ormajor
features