Skip to content

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Aug 16, 2023

Summary

Added the ability to customize which headings will be pulled into the in-page navigation link list.
Use the new data-heading-elements attribute to designate the heading levels that should be included in the component.

The headings should be listed with a space in between. Here is a sample implementation:

<aside 
    class="usa-in-page-nav" 
    data-heading-elements="h3 h4"
</aside>

If this attribute is not added to the component or has an empty value, it will default to pulling h2 and h3 headers.

Breaking change

This is not a breaking change.

Related issue

Closes #5030

Related pull requests

Changelog PR

This work originated in #5034.

Preview link

In-page navigation - custom header selector test

Problem statement

The in-page navigation component is hard-coded to pull all h2 and h3 headers from the designated main content region. However, there are valid use cases where teams would want to control which headings will be included in their component.

Solution

Creating data-heading-elements attribute will allow users select which header levels they want to include in the component link list. This attribute can accept multiple values (between h1-h6), each separated by a space. If the attribute is not added or is left blank, the component will pull the default h2 and h3 headers.

Testing and review

  • Confirm the attribute name is intuitive and follows convention
  • Confirm the correct headings are pulled when the data-heading-elements attribute is present
  • Confirm that bolded text styles are added to the top-level header in the link list. All other headers in the link list should receive a normal font weight.
  • Confirm code follows convention
  • Confirm the storybook controls make sense

To test in Storybook:

  1. Open the In-page navigation - test custom header selector page
  2. In the controls, choose different selections in the "Include these headers in link list" control. Reload the page after each selection change.
  3. Confirm that only the expected headers are pulled into the in-page navigation link list.
    1. "Default" should pull h2 and h3 headers
    2. "All" should pull h1-h6 headers
    3. "Error" should show an error in the console and not add any headers to the link list
    4. "Empty" should pull h2 and h3 headers
    5. Single header types should pull only the selected header type
  4. Confirm that bolded text styles are added to the highest-level header in the link list. All other headers should have a normal font weight.

To test in a project:

  1. Install this branch and copy the updated JS into your project
  2. Add the following test markup
    <div class="usa-in-page-nav-container">
      <aside class="usa-in-page-nav"
        data-heading-elements="h2">
      </aside>
      <main>
        <h1>H1 heading</h1>
        <h2>H2 heading</h2>
        <h3>H3 heading</h3>
        <h4>H4 heading</h4>
        <h5>H5 heading</h5>
        <h6>H6 heading</h6>
      </main>
    </div>
  3. Change the value of data-heading-elements to any mix of headings and confirm that the link list pulls the correct headings.

Comment on lines -3 to -7
data-title-text="On this page"
data-title-heading-level="h4"
data-scroll-offset="0"
data-root-margin="0px 0px 0px 0px"
data-threshold="1"
Copy link
Contributor Author

@amyleadem amyleadem Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed these attributes so that the component uses defaults. The aim was to improve clarity in testing.

@amyleadem amyleadem marked this pull request as ready for review August 23, 2023 23:07
@amyleadem amyleadem requested review from mejiaj and mahoneycm August 23, 2023 23:07
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loving this change! Great work 👍

  • Attribute name is intuitive and follows convention
  • Correct headings are included
  • Links are styled appropriately
  • Test various heading levels in sandbox
    • h2 h3 h4
    • h2 h4
    • h4 h2
    • h3
  • Confirmed highest level heading is respected despite order in data attribute
  • JS matches USWDS standard
  • Automated tests pass

@smustgrave
Copy link
Contributor

Following

amyleadem added 6 commits January 8, 2024 09:32
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Standardized the visual presentation of the expectations/instructions in this test to match the custom header test.

@amyleadem
Copy link
Contributor Author

@mejiaj I believe I have addressed all of your questions/comments. Please let me know if you need anything else.

@amyleadem amyleadem requested a review from mejiaj January 8, 2024 19:10
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.

Just sound small outstanding items:

  1. Note on BEM I missed on first round (sorry)
  2. Please add the release note in the PR description with the bold text + description.
  3. Old question in previous review.
  4. Question on sanitization in JS.

@amyleadem
Copy link
Contributor Author

@mejiaj Thanks for catching these. Here are my responses to your items:

  1. Note on BEM I missed on first round (sorry)
  2. Please add the release note in the PR description with the bold text + description.
    • Added
  3. Old question in previous review.
  4. Question on sanitization in JS.

Let me know if you see anything else.

@amyleadem amyleadem requested a review from mejiaj January 9, 2024 23:04
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, minor note on nonexistent headers.

Testing items

  • Single header values like h1
  • Several headers like h1, h2, h3
  • Specifying headers that don't appear ex: h6. Empty nav is generated, see comment below.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissing previous review because issue with empty nav is captured in #5610.

Copy link
Contributor

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to approve this, but I think there's a modest usability issue here since there's no distinction between the "top" heading level, and all the "lower" heading levels. But that could be a issue for another day

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USWDS - Feature: Add customizable data- attribute for in-page nav to specify which heading tags should be used
5 participants