-
Notifications
You must be signed in to change notification settings - Fork 139
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: Update header levels on component pages #2608
Conversation
…ds-site into al-variant-header-levels
…riant-header-levels
erge branch 'al-in-page-nav-headers' of https://github.com/uswds/uswds-site into al-variant-header-levels
…riant-header-levels
…riant-header-levels
…ds-site into al-variant-header-levels
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.
@amyleadem - asking you to take a look at just a couple of things. Thanks for making all of these updates!
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! A couple comments that shouldn't block this work
Testing Checklist
- Confirm that the heading structure on the affected component pages is logical.
- Confirm that the in-page navigation link list includes expected headers.
- Confirm that the in-page navigation link list is not too long.
- Confirm no visual regressions on component previews
- Confirm no visual regressions on page content
- Confirm no regressions on the header structure on other component pages.
- Confirm the code meets standards
@mahoneycm, I answered your comments and gave the header naming another try in 1efb7b5. I tried using "top", "second", and "third" instead. I'm hoping that is simpler and clearer. Curious what you think! |
…ds-site into al-variant-header-levels
…riant-header-levels
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.
@amyleadem - I discussed with @finekatie, and we think that for the summary box and validation components the out-of-context h3s in the in-page nav are a little confusing. We are proposing an h3 at the top of each of these component previews that reads "Component example" to resolve the issue.
- This is a stop gap to make the in-page nav list more logical
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.
Looking at the preview links in table, it's not immediately clear what this PR is about. Since the pages look the same.
The before/after comparisons look the same except in-page navigation.
Overall, code looks good. Two small requests.
Question on visual display of headers and its impact on semantics. For example, the a11y impact of h3
and h4
looking the same and its impact on semantics (if any).
Also comment on class for headers in SCSS.
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 - including return to "Default" language in component previews for summary box and validation components (as discussed yesterday). Thanks, @amyleadem!
@mejiaj I made the requested naming updates. I also spoke to the a11y team about your open question, and they didn't see any accessibility risks there. This should be ready for your re-review. Also, do you have any thoughts on @mahoneycm's question about 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.
LGTM, thank you
I tested:
- No visual regressions
- No a11y regressions
- Code looks good & follows standards
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.
Lgtm!
Hey @amyleadem , I took myself off as a reviewer as @sarah-sch has already covered content. I think this is good to go, if and when you're ready for FFR. |
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!
Merging over circle crash |
Summary
Updated the header structure on component pages.
Related issue
Closes #2601
Related PR
Follow-up to #2595
Preview links
Updates to guidance header structure
Note: In these pages, I updated the header structure in
component.html
. The heading levels on these pages should now be nested one level deeper (h2
➡️h3
, etc). However, the styles were adjusted so there should be no visual changes.Updates to component preview only
Note: In these pages, I updated the header structure in the component in the component preview. There should be no visual changes, but the component headers in the component preview accordion should be nested one level deeper. They should not appear in the in-page navigation.
Updates to in-page nav contents only
Note: In these pages, I adjusted which heading levels should be included in the in-page navigation.
Problem statement
On component pages with multiple variants, the component code, component preview, component guidance, and component package headers are on the same heading level as the parent variant header. It is not logical for these to be at the same level as the heading type--they belong under the heading type.
Solution
This PR updates the header levels on our component pages.
level
attribute on thecomponent.html
include to set the heading level for the guidance section.Testing and review
Dev