feat: in-page navigation #2551
feat: in-page navigation #2551
Conversation
werdnanoslen
requested review from
gidjin,
haworku,
ahobson and
brandonlenz
as code owners
| args: { | ||
| headingLevel: 'h4', | ||
| rootMargin: '0px 0px 0px 0px', | ||
| scrollOffset: '0rem', |
There was a problem hiding this comment.
question(scrollOffset): USWDS defines this as
Number of pixels scroll should offset from the top of the parent element.
Should we expect anything other than a flat number of pixels then?
| scrollOffset: '0rem', | |
| scrollOffset: '0', |
There was a problem hiding this comment.
Either way, 0px, 0rem, etc. are best written as 0 for clarity, so I recommend making this change even if nothing else is updated to expect numeric values.
With the way this gets piped in to the css module, it makes sense as a string since 0 is valid but 1 would need to be 1px, which is just a whole mess, so I'm good with it otherwise remaining string-like
There was a problem hiding this comment.
I personally would prefer the flexibility in spacing units rather than strictly to px, which this approach still supports. I did change the story to use just "0", though.
Should we write up a DR on implementation details like this and the offset decision above? My approach is generally to start with the original implementation and then deviate only if improves eng experience or accessibility and is still compatible with the original implementation.
|
I keyboard tested this against Trusted Tester standard Check 2.4.3-focus-order-reveal, and I am not sure we are getting the expected behavior. In USWDS core, their demo shifts the focus to the header that was navigated to when the link is activated in the navigation.
We are currently resetting our focus when the link is activated. We probably want to replicate the behavior of the original component in USWDS, which I'd hope was tested with users and validated as the best way to provide an accessible AT experience.
tested in Chrome on Mac |
Oh great catch! I'll try to get around to that unless you've got a fix in mind? |
|
Ah, I had just omitted the tabindexes from the sample content. Should work as expected now |
|
Hello. I will do a design review on this now. |
- added mainProps for customization of <main> element - moved usa-prose class to story as example of mainProps - made offsetStyle conditional on scrollOffset being set
There was a problem hiding this comment.
I do have some concerns about this component implicitly creating the <main> tag.
It might keep it from being as flexible as we want it to be. Having the component is definitely better than not having it though, and knowing that consumers need it, and are okay with this iteration, let's merge. If anything we can build on this and refactor it if folks end up preferring more flexibility
Summary
Adds in-page navigation component
Related Issues or PRs
Resolves #2518
How To Test
Storybook
Screenshots (optional)