-
Notifications
You must be signed in to change notification settings - Fork 80
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
feat: in-page navigation #2551
feat: in-page navigation #2551
Conversation
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.
A few things here, let me know if you want assistance making any of these changes, or if anything is unclear!
args: { | ||
headingLevel: 'h4', | ||
rootMargin: '0px 0px 0px 0px', | ||
scrollOffset: '0rem', |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Hello. I will do a design review on this now. |
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.
No happo diffs 🚀. Great work.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
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 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)