USWDS - In-page navigation: Add an option to designate the main content element #5387

amyleadem · 2023-07-17T21:25:20Z

Summary

Added the optional data-main-content-selector attribute to the in-page navigation component. This attribute allows users to designate which element they want the component to pull headers from. If the attribute is not defined, the component will pull headers from the <main> element.

Breaking change

This is not a breaking change.

Related issue

Closes #5050

Related pull requests

Changelog and documentation PR

Note
This issue was originally worked in #5124.

The edits in this new PR address only the ability to select a custom content region. We will need to discuss if we want to incorporate the other work included in this original PR, namely the changes to the component unit tests.

Preview link

In-page navigation

Problem statement

In its current state, the in-page navigation component is hard-coded to pull headers from the <main> element. Users are not able to customize where the component will look for headers.

Solution

This PR creates the option to designate which element the in-page nav will pull headers from. It does this by creating the option to include a data-main-content-selector attribute on the usa-in-page-nav element. This attribute can accept both class and id values. If the data-main-content-selector attribute is not defined, the component will pull from the <main> element.

Example usage with a class selector:

Note
Using a class selector will allow for the component to pull from multiple content regions on the same page

<aside class="usa-in-page-nav" 
    data-main-content-selector=".main-content">
</aside>
<main>
    <h2>This header will not be included in the component link list</h2>
    <section class="main-content">
      <h2>This header will be included in the component link list</h2>
    </section>
</main>

Example usage with an id selector:

<aside class="usa-in-page-nav" 
    data-main-content-selector="#main-content">
</aside>
<main>
    <h2>This header will not be included in the component link list</h2>
    <section id="main-content">
      <h2>This header will be included in the component link list</h2>
    </section>
</main>

Testing and review

Confirm that the data-main-content-selector attribute name is intuitive
Confirm that adding a custom value to the data-main-content-selector attribute pulls the expected headings
Let me know if this should have a unit test (I might need to pair up for that)

To test in Storybook:

Open the Test Custom Content Selector story. Confirm that only this link appears in the in-page navigation:
Set the customContentSelector control to false and reload the page. Confirm that both links are shown in the in-page navigation:

To test in a project:

Install this branch
Add in-page navigation component code to a test page.
Create a custom main content region that you want the component to pull headers from
Add the data-main-content-selector to the usa-in-page-nav element and define it with the id or class from the custom content region
Confirm that the in-page navigation only pulls headers form the designated content region

Credits

Thanks to @aduth for the suggestion in this comment on PR #5124

- Allows users to set the element where the component pulls headers

- Make the name more intuitive

- Will be addressed separately in issue #5392

- Align testing technique with content selector test in #5387

mejiaj

Nice work so far!

Since we use querySelectorAll() having a duplicate ID or class will generate the nav with their respective headers. Meaning, you could end up with seemingly more content than originally intended. This doesn't seem like a blocker.

Added suggestions below. Can we also include a basic unit test to check for a custom ID & class?

packages/usa-in-page-navigation/src/index.js

packages/usa-in-page-navigation/src/usa-in-page-navigation.stories.js

to add clarity of purpose - Also Improve JSDoc comments for getSectionHeadings

…age-nav-parent

amyleadem

@mejiaj I have updated the unit tests according to our conversation. I'm hoping the description and naming make sense, but let me know if you see any opportunities for improvement. Thanks!

packages/usa-in-page-navigation/src/test/test-pattern/test-custom-content-selector.twig

packages/usa-in-page-navigation/src/usa-in-page-navigation.stories.js

packages/usa-in-page-navigation/src/test/template.html

packages/usa-in-page-navigation/src/test/in-page-navigation.spec.js

mahoneycm

@amyleadem thank you for that tip! Working like a charm now!

This PR looks good to me!

Including the attribute works as expected
Links are included or excluded based on the attribute's value
Works for both ID's and classes
Works with multiple divs using the same class
Test is accurate and functions properly

mejiaj

Almost there, just a small change to split this new feature into its own unit test + template.

I also added HTML syntax highlighting to code examples under Solution section for readability.

packages/usa-in-page-navigation/src/test/template.html

amyleadem · 2023-08-08T18:12:31Z

@mejiaj I have broken the unit tests for this PR into their own files. Will you let me know if you see any opportunities for improvement?

mejiaj

Just some minor changes, otherwise LGTM.

packages/usa-in-page-navigation/src/test/test-pattern/test-custom-content-selector.twig

packages/usa-in-page-navigation/src/test/in-page-nav-custom-content.spec.js

mejiaj · 2023-08-10T14:27:36Z

packages/usa-in-page-navigation/src/test/in-page-nav-custom-content.spec.js

+];
+
+tests.forEach(({ name, selector: containerSelector }) => {
+  describe(`in-page navigation initialized at ${name}`, () => {


The describe on line 22 should be updated to match what we're testing.

Currently it's hard to tell what's being tested. Here's the output:

You can see at a glance it's hard to tell since they both use the same describe() - in-page navigation initialized at document.body shows twice.

Thanks! Updated in 394a463.

mejiaj · 2023-08-10T15:42:52Z

packages/usa-in-page-navigation/src/test/in-page-nav-custom-content.spec.js

+      const customRegionLink = listLinks.filter((link) =>
+        link.href.includes("#header-in-content-region")
+      );
+      assert.equal(customRegionLink.length === 1, true);


It might be more reliable to just check the nav link length matches the header in content region length?

For example, it fails if you add more headers.

Instead of updating the test highlighted here, I updated the first unit test that checked for exactly 1 link in the link list in this commit. I might have misunderstood the request, so let me know if wanted me to change this one instead.

mejiaj · 2023-08-10T16:22:17Z

packages/usa-in-page-navigation/src/test/in-page-nav-custom-content.spec.js

+
+    it("does not create a link in the nav list for the header that is outside the custom content region", () => {
+      const mainRegionLink = listLinks.filter((link) =>
+        link.href.includes("#header-not-in-content-region")


Note
If I delete the header outside of the content region this test still passes. This is expected, but could cause confusion.

amyleadem · 2023-08-10T21:39:24Z

@mejiaj thanks for all the coaching on this. I am leveling up on unit tests - I can feel it! Let me know if you want any other changes.

PS - I also updated the test-pattern twig file to remove the macro like we did on the hidden headers PR (#5393) in 9d319a1.

amyleadem · 2023-08-11T16:50:36Z

FYI - Working on merging in the changes from #5393

…age-nav-parent

amyleadem · 2023-08-11T17:01:09Z

@mejiaj I resolved all merge conflicts. Can you take a look at the recent changes when you get a chance?

mejiaj

LGTM! The unit tests are clear and consistent with current patterns.

Unit test output

Add an option for main-selector data attribute

3e38d78

- Allows users to set the element where the component pulls headers

amyleadem changed the title ~~USWDS - In-page navigation: Add an option to designate a custom main element~~ USWDS - In-page navigation: Add an option to designate the main content element Jul 17, 2023

amyleadem mentioned this pull request Jul 17, 2023

USWDS-Site - Changelog: Update documentation and add entry for in-page nav custom content [USWDS#5387] uswds/uswds-site#2190

Merged

amyleadem added 13 commits July 17, 2023 15:15

Change mainSelector to mainContentSelector

5016e1b

- Make the name more intuitive

Filter out hidden headings

1d6061c

Run prettier and add code comments

e47c3ce

Add Storybook test for data-main-content-selector

355c834

Run prettier

c676bcb

Add code comment for second main-content region

28ca2ab

Remove code related to hidden headers

dfb7254

- Will be addressed separately in issue #5392

Revert in-page nav twig

a84bcea

Add storybook tests for data-main-content-selector

b6ae998

Format test twig

4b48ce7

Fix storybook customContentSelector control

d9e13d2

Run prettier

b25ad10

Replace TestHiddenHeader with TestCustomContent

6cb5dc0

amyleadem added a commit that referenced this pull request Jul 19, 2023

Improve Storybook tests

67dbc9e

- Align testing technique with content selector test in #5387

amyleadem mentioned this pull request Jul 19, 2023

In-page nav main content section requirement changed from the main element to a class #5124

Closed

amyleadem added 2 commits July 20, 2023 13:54

Add explanations to custom content test template

4e57798

Improve text in test intro

86c799e

amyleadem marked this pull request as ready for review July 21, 2023 20:16

amyleadem requested review from mejiaj and mahoneycm July 21, 2023 20:16

mejiaj requested changes Aug 1, 2023

View reviewed changes

packages/usa-in-page-navigation/src/index.js Outdated Show resolved Hide resolved

packages/usa-in-page-navigation/src/index.js Outdated Show resolved Hide resolved

packages/usa-in-page-navigation/src/usa-in-page-navigation.stories.js Outdated Show resolved Hide resolved

amyleadem added 6 commits August 1, 2023 09:36

Pass mainContentSelector to SectionHeadings param

d8a55c6

to add clarity of purpose - Also Improve JSDoc comments for getSectionHeadings

Simplify + rename main content selector test story

107444c

Merge branch 'develop' of https://github.com/uswds/uswds into al-in-p…

8d71259

…age-nav-parent

Update control name to customContentSelector

af1e4f5

Run prettier

bd41733

Add unit test for data-main-content-selector

31de2c5

amyleadem requested a review from mahoneycm August 7, 2023 23:11

amyleadem commented Aug 7, 2023

View reviewed changes

mahoneycm approved these changes Aug 8, 2023

View reviewed changes

amyleadem mentioned this pull request Aug 8, 2023

USWDS-Site: Documentation updates for release 3.5.1 uswds/uswds-site#2223

Closed

mejiaj requested changes Aug 8, 2023

View reviewed changes

packages/usa-in-page-navigation/src/test/template.html Outdated Show resolved Hide resolved

amyleadem added 7 commits August 8, 2023 10:49

Revert in-page-navigation.spec.js

ce40093

Revert in-page-nav default test template

e3f8d71

Create test for custom content region

890997f

Run prettier

17375c4

Format code

1efe2dc

Format code

7f4057d

Simplify comment

2568c34

amyleadem requested a review from mejiaj August 8, 2023 18:12

mejiaj requested changes Aug 10, 2023

View reviewed changes

amyleadem added 4 commits August 10, 2023 09:35

Rename custom content test files

924fd2a

Make unit tests more resilient

394a463

Run prettier

b050bc0

Remove macro on test pattern

9d319a1

amyleadem requested a review from mejiaj August 10, 2023 21:39

Merge branch 'develop' of https://github.com/uswds/uswds into al-in-p…

16a0c7a

…age-nav-parent

mejiaj approved these changes Aug 11, 2023

View reviewed changes

mejiaj requested a review from thisisdano August 11, 2023 17:45

thisisdano merged commit 4fe8dda into develop Aug 18, 2023
5 checks passed

thisisdano deleted the al-in-page-nav-parent branch August 18, 2023 16:49

thisisdano mentioned this pull request Aug 18, 2023

Release 3.6.0 #5454

Merged

amyleadem mentioned this pull request Nov 13, 2023

In-page nav enhancements #5105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USWDS - In-page navigation: Add an option to designate the main content element #5387

USWDS - In-page navigation: Add an option to designate the main content element #5387

amyleadem commented Jul 17, 2023 •

edited

Loading

mejiaj left a comment •

edited

Loading

amyleadem left a comment

mahoneycm left a comment

mejiaj left a comment

amyleadem commented Aug 8, 2023

mejiaj left a comment

mejiaj Aug 10, 2023

amyleadem Aug 10, 2023 •

edited

Loading

mejiaj Aug 10, 2023

amyleadem Aug 10, 2023

mejiaj Aug 10, 2023

amyleadem commented Aug 10, 2023

amyleadem commented Aug 11, 2023

amyleadem commented Aug 11, 2023

mejiaj left a comment

USWDS - In-page navigation: Add an option to designate the main content element #5387

USWDS - In-page navigation: Add an option to designate the main content element #5387

Conversation

amyleadem commented Jul 17, 2023 • edited Loading

Summary

Breaking change

Related issue

Related pull requests

Preview link

Problem statement

Solution

Testing and review

Credits

mejiaj left a comment • edited Loading

Choose a reason for hiding this comment

amyleadem left a comment

Choose a reason for hiding this comment

mahoneycm left a comment

Choose a reason for hiding this comment

mejiaj left a comment

Choose a reason for hiding this comment

amyleadem commented Aug 8, 2023

mejiaj left a comment

Choose a reason for hiding this comment

mejiaj Aug 10, 2023

Choose a reason for hiding this comment

amyleadem Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

mejiaj Aug 10, 2023

Choose a reason for hiding this comment

amyleadem Aug 10, 2023

Choose a reason for hiding this comment

mejiaj Aug 10, 2023

Choose a reason for hiding this comment

amyleadem commented Aug 10, 2023

amyleadem commented Aug 11, 2023

amyleadem commented Aug 11, 2023

mejiaj left a comment

Choose a reason for hiding this comment

amyleadem commented Jul 17, 2023 •

edited

Loading

mejiaj left a comment •

edited

Loading

amyleadem Aug 10, 2023 •

edited

Loading