-
Notifications
You must be signed in to change notification settings - Fork 138
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
USWDS-Site: Restore in-page navigation on long pages #2595
Conversation
_layouts/styleguide.html
Outdated
@@ -17,7 +17,9 @@ | |||
aria-label="On this page" | |||
data-scroll-offset="-120" | |||
data-root-margin="48px 0px -90% 0px" | |||
data-threshold="1"></aside> | |||
data-threshold="1" | |||
data-heading-elements={{ page.in_page_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.
Devs, I considered adding a check here for certain values, but ultimately moved away from this because it felt unnecessarily constraining. Let me know if you think I should add something like this in.
data-heading-elements={{ page.in_page_nav }} | |
{% if page.in_page_nav == "h2" or page.in_page_nav == "h3" %} | |
data-heading-elements={{ page.in_page_nav }} | |
{% endif %} |
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.
What would this change do?
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 update shown in the suggested change would just prevent the attribute from being added unless the variable receives one of the expected values.
The benefit of this is that the rendered code would be cleaner (the attribute wouldn't be added with an empty value).
The downside of this is that it either limits the possible values we can enter OR we'd have to check for all possible combinations here. For example, maybe we will have a need to show h3
and h4
values in the in-page nav on a given page, or maybe we would want h2
, h3
, and h4
.
I opted for an open value for the attribute here for maximum flexibility.
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 is looking good! While testing I noticed a few visual discrepancies that might be out of scope for this issue but I wanted to document
Testing checklist
- Confirm that the in-page navigation component shows up on the pages listed in "Preview links" above
- Confirm that the pages that need an in-page navigation component have one
- Confirm that there are no regressions with the in-page nav on other pages
- Confirm that the code added to styleguide.html meets code standards
Additional findings
Deactivate in-page nav on What is web performance
I noticed this page has an empty in-page navigation that should be removed. It's a small change but we could also add it to #2543 or create another issue if it's out of scope for this ticket.
Add setting to select breakpoint that activates in-page nav
This is a larger lift that should be documented in a new issue if we choose to implement it. Currently, the in-page nav component is set to activate at tablet
widths. I noticed the display of the component overview page starts to break just over the tablet
viewport.
What do you think about adding an additional attribute or theme setting to update the breakpoints for in-page navs?
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.
Requesting some changes. Some of these may be better handled in a new issue, so just let me know if I can help write one. I do not have any additional pages to add--you've covered the ones I had noted as needing in-page-nav in the audit. Thank you!
@sarah-sch I have made the requested updates and opened a new issue (#2601) to address the component page's header structure. This is ready for your re-review. |
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.
Thanks, @amyleadem! As long as we can work towards improving the nesting of headings on those longer component pages I think these are good to publish.
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.
I was able to confirm in-page nav is now displaying correctly on these pages. Minor suggestion on variable name.
If you disagree with it, please comment and we can resolve.
Pages tested
Page | Old | New |
---|---|---|
Accessibility | Live → | Preview → |
Header | Live → | Preview → |
Language selector | Live → | Preview → |
Migrating | Live → | Preview → |
Settings | Live → | Preview → |
_layouts/styleguide.html
Outdated
@@ -17,7 +17,9 @@ | |||
aria-label="On this page" | |||
data-scroll-offset="-120" | |||
data-root-margin="48px 0px -90% 0px" | |||
data-threshold="1"></aside> | |||
data-threshold="1" | |||
data-heading-elements={{ page.in_page_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.
What would this change do?
- This will make the name of the data key more intuitive
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.
Note
There's an HTMLProofer error resolved by #2604.
Thanks for updating, approved.
…-page-nav-headers
…-page-nav-headers
…-page-nav-headers
…-page-nav-headers
…-page-nav-headers
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.
Thank you!
Summary
in_page_nav
front matter variable name toin_page_nav_headings
. This will improve clarity about the intent of the variable.New value options
The
in_page_nav
front matter data key now accepts header levels in its value definition. If the value is anything other thanfalse
, it will add that value to the in-page navigation'sdata-heading-elements
attribute, as documented on our website. Ifin_page_nav
is not defined, the in-page navigation component will automatically be added to the page and will show bothh2
andh3
headers in the link list.Refer to the table below for expected implementations of
in_page_nav
:h2
headers in the page's in-page navigation link list. This should be used when the page's heading structure is very long and causes the link list to extend beyond the viewport.h3
headers in the page's in-page navigation link list. This use case will be rare.Related issue
Closes #2594
Preview link
Problem statement
Previously, we removed the in-page navigation component from pages with a long list of headers that caused the component to beyond the viewport. We should use the new
data-heading-elements
attribute on the in-page navigation component to show onlyh2
elements in the link list.Solution
Adding the option to show only
h2
headers on the in-page navigation component instead of the defaulth2
andh3
headers allows us to shorten the in-page navigation link list and display the component on the page.Testing and review
Devs