-
Notifications
You must be signed in to change notification settings - Fork 233
docs(topnav): top nav docs accessibility audit #5724
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?
docs(topnav): top nav docs accessibility audit #5724
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... |
packages/top-nav/README.md
Outdated
</sp-tab-panel> | ||
</sp-tabs> | ||
|
||
#### Other properties |
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.
Any thoughts on a different heading for this? I tried to keep some of the original examples, but it ended up being sort of hodge-podge of examples to lump together (hence "other properties").
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.
Hmmm, you could move this <sp-tabs>
up above sizes, leave it without a heading, then put Sizes down below with the #### Sizes
heading. But it's also fine as it is.
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.
ah yes! I think I like this better- thanks!
e36bd60
to
49db7c8
Compare
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 looks great! I called out a few things, mostly questions that may or may not need addressing, plus some syntax-y type things, I wouldn't consider any of that really blocking so I'm going to approve. Happy to take another look at this after you make changes though, if you want!
packages/top-nav/README.md
Outdated
### Example | ||
### Anatomy | ||
|
||
The `sp-top-nav`` component consists of the following parts: |
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.
The `sp-top-nav`` component consists of the following parts: | |
The `sp-top-nav` component consists of the following parts: |
<sp-top-nav-item href="#pam">Pam</sp-top-nav-item> | ||
<sp-top-nav-item href="#phyllis">Phyllis</sp-top-nav-item> | ||
<sp-top-nav-item href="#angela" disabled>Angela</sp-top-nav-item> | ||
<sp-top-nav-item href="#meredith">Meredith</sp-top-nav-item> |
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.
😆
The `sp-top-nav-item` consists of the following parts: | ||
|
||
- **Default slot**: text content for the navigation item | ||
- **Label**: Sets a visually-hidden `aria-label` on the item |
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 wanted to ask about this one. At first, I was wondering if we should set label
on some of the examples here, but then I saw the best practices note saying to use label
sparingly. But it also says you can use it if "screen reader text is significantly different from visible text," so maybe it's useful instead to show an example where that's used? Maybe an icon-only item?
packages/top-nav/top-nav-item.md
Outdated
|
||
### Anatomy | ||
|
||
The `sp-top-nav-item` consists of the following parts: |
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.
Do we want brackets? I think there are other instances of this happening, but I won't point out all of them since I know left a lot of comments already on the sidenav PR.
The `sp-top-nav-item` consists of the following parts: | |
The `<sp-top-nav-item>` consists of the following parts: |
packages/top-nav/top-nav-item.md
Outdated
```html | ||
<sp-top-nav | ||
selected="https://opensource.adobe.com/spectrum-web-components/components/top-nav-item/#dwight" | ||
> | ||
<sp-top-nav-item href="#michael">Michael</sp-top-nav-item> | ||
<sp-top-nav-item href="#dwight" selected>Dwight</sp-top-nav-item> | ||
<sp-top-nav-item href="#kevin">Kevin</sp-top-nav-item> | ||
<sp-top-nav-item href="#jim">Jim</sp-top-nav-item> | ||
</sp-top-nav> | ||
``` |
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.
When I load the page, I don't see Dwight selected. Is that expected, because the selected url in <sp-top-nav>
doesn't match the preview url?
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.
packages/top-nav/README.md
Outdated
Home | ||
</sp-top-nav-item> | ||
<sp-top-nav-item href="#services">Services</sp-top-nav-item> | ||
<sp-top-nav-item href="#about">About</sp-top-nav-item> |
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.
Does it matter if this item doesn't have selected
?
packages/top-nav/README.md
Outdated
</sp-tab-panel> | ||
</sp-tabs> | ||
|
||
#### Other properties |
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.
Hmmm, you could move this <sp-tabs>
up above sizes, leave it without a heading, then put Sizes down below with the #### Sizes
heading. But it's also fine as it is.
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 have a few changes and some questions.
packages/top-nav/README.md
Outdated
### Example | ||
### Anatomy | ||
|
||
The `sp-top-nav`` component consists of the following parts: |
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.
The `sp-top-nav`` component consists of the following parts: | |
The `<sp-top-nav>` component consists of the following parts: |
packages/top-nav/README.md
Outdated
</sp-tab-panel> | ||
</sp-tabs> | ||
|
||
#### Other properties |
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 noticed a selectionIndicatorStyle
property in the API(https://swcpreviews.z13.web.core.windows.net/pr-5724/docs/components/top-nav/api/#attributes-and-properties). We should probably include information about it.
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 have some questions about that property! I can definitely include more info on it, but I'm unsure how to use it properly.
Because this property is tied to the window.location.href, when I try to manually set it, it automatically gets reset to noSelectionStyles
since the URL of the docs page doesn't actually match the selected URL I defined in the demo. I think I have fixed up the examples to properly show the selection indicator once this is merged, (for example, when you run this locally, use the localhost:8080
URL in place of opensource.adobe.com/spectrum-web-components
), but I can't quite figure out how to "use" selectionIndicatorStyle
.
packages/top-nav/README.md
Outdated
Home | ||
</sp-top-nav-item> | ||
<sp-top-nav-item href="#services">Services</sp-top-nav-item> | ||
<sp-top-nav-item href="#about">About</sp-top-nav-item> |
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.
The selection indicator is not showing here. Does it require a selectionIndicatorStyle
value?
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.
The selection indicator is only triggered after a selection is made. I believe it's JS only. To be frank, I'm having a tough time figuring out how to even document this, as it seems like it just keeps getting overridden.
<sp-top-nav> | ||
<sp-top-nav-item href="#pam">Pam</sp-top-nav-item> | ||
<sp-top-nav-item href="#phyllis">Phyllis</sp-top-nav-item> | ||
<sp-top-nav-item href="#angela" disabled>Angela</sp-top-nav-item> | ||
<sp-top-nav-item href="#meredith">Meredith</sp-top-nav-item> |
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.
Love this!
packages/top-nav/top-nav-item.md
Outdated
selected="https://opensource.adobe.com/spectrum-web-components/components/top-nav-item/#dwight" | ||
> | ||
<sp-top-nav-item href="#michael">Michael</sp-top-nav-item> | ||
<sp-top-nav-item href="#dwight" selected>Dwight</sp-top-nav-item> |
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.
The selection indicator is not showing here. Does the <sp-top-nav>
require a selectionIndicatorStyle
value?
Description
This PR adds to the
sp-top-nav
andsp-top-nav-item
component documentation, with more complete API documentation, accessibility guidance, and practical usage examples.Motivation and context
The existing sidenav documentation was incomplete and lacked API coverage, and did not follow SWC's recommended format. This work should also now make the documentation accessible for future audits.
Related issue(s)
Screenshots (if appropriate)
N/A - Documentation changes only
Author's checklist
I have added automated tests to cover my changes.I have included a well-written changeset if my change needs to be published.Reviewer's checklist
Includes thoughtfully written changeset if changes suggested includepatch
,minor
, ormajor
featuresAutomated tests cover all use cases and follow best practices for writingAll VRTs are approved before the author can update Golden HashManual review test cases
Complete API documentation verification
Accessibility documentation validation
Example accessibility validation
Device review