[IDP] In-page navigation #4918

mitchmoccia · 2022-08-15T19:26:56Z

This is a follow-up PR for In-page navigation from this earlier PR:
#4860

https://federalist-3b6ba08e-0df4-44c9-ac73-6fc193b0e19c.app.cloud.gov/preview/uswds/uswds/mm-in-page-nav4/?path=/story/components-in-page-navigation--default

This version addresses all the great comments left by @mejiaj. Looking forward to any and all feedback, thank you!

packages/usa-in-page-navigation/src/styles/_usa-in-page-navigation.scss

amyleadem · 2022-08-17T21:23:32Z

@mitchmoccia Starting to do some testing on appearance and behavior - it is working well so far! I had a few questions about alignment and am flagging some questions in the code comments. I will keep looking poking at it this afternoon and will cycle back tomorrow for more review. If you have any questions in the meantime, please let me know!

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

amyleadem · 2022-08-18T13:53:32Z

packages/usa-in-page-navigation/src/usa-in-page-navigation.twig

+    {% endif %}
+  </main>
+
+  <aside class="usa-in-page-nav"></aside>


@mitchmoccia @mejiaj
Wondering about the benefits of nesting the <nav> inside an <aside>. I believe it is valid (depending on how it is defined), but feels semantically unnecessary and appears to just add an extra layer for users to tab through.

Can we just make this element the <nav> itself or switch to a <div> (if it needs a wrapper)?

The HTML5 standard mentions [html.spec.whatwg.org]:

The element can be used for typographical effects like pull quotes or sidebars, for advertising, for groups of nav elements, and for other content that is considered separate from the main content of the page.

And they have an example below (third example down):

TL;DR I can see both sides of the argument. Semantically I think it's okay. The final decision depends on the screen reader experience. Unless others understand the use case better, please chime in!

Makes sense!
Something I noticed is that in the same spec, it does show this example for the nav element. https://html.spec.whatwg.org/multipage/sections.html#the-nav-element

In the following example, there are two nav elements, one for primary navigation around the site, and one for secondary navigation around the page itself.

FWIW: I did poke around at other implementations and found that in its own implementation of a similar component (The "In this article" TOC on the right), Mozilla uses an <aside> with a nested <section> and no <nav>: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/aside

Other popular design systems use <nav> with a nested list or just a <div> with a nested list.

So my tl;dr is that it seems like there are a lot of possible correct paths here regarding if it is a nav or aside (or both), and that it might just depend on how we define "tangential" or "indirectly related" content. Users should still be able to find what they need with either. Change would only be needed here if we want to remove an extra layer when users navigate by element, but it shouldn't affect users who tab into it.

Reference:

The aside element represents a section of a page that consists of content that is tangentially related to the content around the aside element, and which could be considered separate from that content. Such sections are often represented as sidebars in printed typography.

The element can be used for typographical effects like pull quotes or sidebars, for advertising, for groups of nav elements, and for other content that is considered separate from the main content of the page.

Love the tag team analysis!! I did some quick testing in VoiceOver and it doesn't seem to make a difference... I did make the decision to use aside > nav based on the w3c example but will go with whatever makes most sense.

packages/uswds-core/src/styles/settings/_settings-components.scss

mejiaj

Very nice rewrite @mitchmoccia! The code, but JS especially looks much more in-line with other component standards.

Added some comments and suggestions. Also, I'm sure you've got this in mind already, but I didn't notice any unit tests for this component.

packages/usa-in-page-navigation/src/styles/_usa-in-page-navigation.scss

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

mejiaj · 2022-08-18T13:57:51Z

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

+    },
+    keydown: {
+      [IN_PAGE_NAV_LINK]: keymap({
+        Enter: handleEnterFromLink,


Triggering via spacebar might also be a good idea, not a requirement though.

Let's consider this as a possible followup enhancement.

Created issue #5021.

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

packages/usa-in-page-navigation/src/styles/_usa-in-page-navigation.scss

packages/uswds-core/src/styles/settings/_settings-components.scss

mitchmoccia · 2022-08-18T16:18:27Z

Amazing feedback @amyleadem and @mejiaj THANK YOU... I'll be addressing all of these ASAP

…page-nav class to prevent appendChild errors on pages that dont have it

… the nav before content when tabbing through the page

mitchmoccia · 2022-11-03T19:40:10Z

@thisisdano @mejiaj @amyleadem Just made a great update per Dan's suggestion... The in-page nav heading text and heading level are now customizable from the HTML via data attributes! The new attributes are data-title and data-heading-level both added to the aside element.

I have also updated the unit tests and everything is looking good. Let me know how it looks on your end, thanks!

…s are not present

mitchmoccia · 2022-11-03T20:03:16Z

I added two variables const
IN_PAGE_NAV_TITLE = “On this page”;
IN_PAGE_NAV_HEADING_LEVEL = “h4"

and then check for the presence of the corresponding data attributes and if none, use the defaults...

mejiaj

Some great improvements here! Added some comments and questions.

I tested the for:

ANDI a11y plugin - no errors
AXE plugin - no errors
Checked display on site - minor with empty <p></p>
Checked output HTML of component - minor with empty <p></p>
Tested custom data attributes for heading
Reviewed JS for issues and possible improvements
SASS formatting and possible improvements
Ran NPM test - It seems only in-page nav tests ran

Seeing the same in CircleCI build.

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

packages/usa-in-page-navigation/src/usa-in-page-navigation.twig

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

packages/usa-in-page-navigation/src/styles/_usa-in-page-navigation.scss

Co-authored-by: James Mejia <james.mejia@gsa.gov>

…tion.scss Co-authored-by: James Mejia <james.mejia@gsa.gov>

Co-authored-by: James Mejia <james.mejia@gsa.gov>

…d in the JSON

…tion.scss Co-authored-by: James Mejia <james.mejia@gsa.gov>

…age-nav4

packages/usa-in-page-navigation/src/styles/_usa-in-page-navigation.scss

Addressed changes

mitchmoccia added 3 commits August 14, 2022 11:58

Integrate in-page nav into fresh clone of uswds

514c465

Integration of in-page nav with fresh clone of USWDS

440d678

Fix formatting of in-page nav twig template

4108c44

mitchmoccia requested review from jonadecker, mejiaj and amyleadem August 15, 2022 19:26

Add new style customization variable for top position of nav when sticky

d60f333

amyleadem reviewed Aug 17, 2022

View reviewed changes

packages/usa-in-page-navigation/src/styles/_usa-in-page-navigation.scss Outdated Show resolved Hide resolved

amyleadem reviewed Aug 17, 2022

View reviewed changes

packages/usa-in-page-navigation/src/styles/_usa-in-page-navigation.scss Outdated Show resolved Hide resolved

amyleadem reviewed Aug 17, 2022

View reviewed changes

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

amyleadem reviewed Aug 17, 2022

View reviewed changes

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

amyleadem reviewed Aug 17, 2022

View reviewed changes

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

amyleadem reviewed Aug 18, 2022

View reviewed changes

packages/uswds-core/src/styles/settings/_settings-components.scss Outdated Show resolved Hide resolved

mejiaj previously requested changes Aug 18, 2022

View reviewed changes

amyleadem reviewed Aug 18, 2022

View reviewed changes

packages/uswds-core/src/styles/settings/_settings-components.scss Outdated Show resolved Hide resolved

amyleadem reviewed Aug 18, 2022

View reviewed changes

packages/uswds-core/src/styles/settings/_settings-components.scss Show resolved Hide resolved

amyleadem reviewed Aug 18, 2022

View reviewed changes

packages/uswds-core/src/styles/settings/_settings-components.scss Outdated Show resolved Hide resolved

mitchmoccia added 8 commits August 19, 2022 08:02

In progress updates from code review

1edc406

Round one of addressing PR feedback

95d3413

Get in-page nav test working with a mock intersection observer

9321b2c

Add unit tests for in-page navigation

233956b

Add selectoOrMatch function to search for the presence of the usa-in-…

9b7b8cf

…page-nav class to prevent appendChild errors on pages that dont have it

Update in-page navigation JSON and test file

18931d9

Make main content max width theme setting name more descriptive

54d9ec1

Change order of in-page nav in the DOM so a keyboard user will access…

d2a260c

… the nav before content when tabbing through the page

thisisdano changed the title ~~In-page navigation complete rewrite~~ [IDP] In-page navigation Nov 2, 2022

thisisdano changed the base branch from develop to idp-staging November 2, 2022 05:16

Create default heading text and heading level for when data attribute…

68a38fc

…s are not present

Prevent reformatting when heading level changes

9ad6c4b

mejiaj reviewed Nov 4, 2022

View reviewed changes

thisisdano and others added 16 commits November 7, 2022 12:25

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

9636a9c

Co-authored-by: James Mejia <james.mejia@gsa.gov>

Update packages/usa-in-page-navigation/src/styles/_usa-in-page-naviga…

ed4e2ea

…tion.scss Co-authored-by: James Mejia <james.mejia@gsa.gov>

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

ddc5cfe

Co-authored-by: James Mejia <james.mejia@gsa.gov>

Add JSDoc documentation to createInPageNav function

fc728e0

Apply prettier to in-page nav scss to fix extra spaces and formatting

f39e37b

In-page nav remove p tags around content since p tags are now include…

eb8248e

…d in the JSON

Use simpler border CSS

f870d9f

Update packages/usa-in-page-navigation/src/styles/_usa-in-page-naviga…

fd351c1

…tion.scss Co-authored-by: James Mejia <james.mejia@gsa.gov>

Scope only to main element

05bcd54

Merge branch 'mm-in-page-nav4' of github.com:uswds/uswds into mm-in-p…

64ef1af

…age-nav4

Add a BEM class to the nav

01e144e

Flatten Sass

731f482

Reorder to reflect markup order

6e2804c

Set width in default

bb62e4a

Typeset only once

c0efdfa

Simplify handleClickFromLink function per James suggestion

ee9c8d3

mitchmoccia commented Nov 8, 2022

View reviewed changes

packages/usa-in-page-navigation/src/styles/_usa-in-page-navigation.scss Show resolved Hide resolved

This was referenced Nov 8, 2022

USWDS - In-page nav: Check if element is in focus on keyboard navigation #5020

Open

USWDS - Feature: in-page navigation links should trigger on spacebar #5021

Closed

thisisdano approved these changes Nov 8, 2022

View reviewed changes

thisisdano merged commit 3155040 into idp-staging Nov 8, 2022

thisisdano deleted the mm-in-page-nav4 branch November 8, 2022 19:31

werdnanoslen mentioned this pull request Jul 24, 2023

New Component: In-page Navigation trussworks/react-uswds#2518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IDP] In-page navigation #4918

[IDP] In-page navigation #4918

mitchmoccia commented Aug 15, 2022

amyleadem commented Aug 17, 2022

amyleadem Aug 18, 2022

mejiaj Aug 18, 2022

amyleadem Aug 18, 2022 •

edited

Loading

mitchmoccia Aug 19, 2022

mejiaj left a comment

mejiaj Aug 18, 2022

thisisdano Nov 7, 2022

mejiaj Nov 8, 2022

mitchmoccia commented Aug 18, 2022

mitchmoccia commented Nov 3, 2022

mitchmoccia commented Nov 3, 2022

mejiaj left a comment •

edited

Loading

[IDP] In-page navigation #4918

[IDP] In-page navigation #4918

Conversation

mitchmoccia commented Aug 15, 2022

amyleadem commented Aug 17, 2022

amyleadem Aug 18, 2022

Choose a reason for hiding this comment

mejiaj Aug 18, 2022

Choose a reason for hiding this comment

amyleadem Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

mitchmoccia Aug 19, 2022

Choose a reason for hiding this comment

mejiaj left a comment

Choose a reason for hiding this comment

mejiaj Aug 18, 2022

Choose a reason for hiding this comment

thisisdano Nov 7, 2022

Choose a reason for hiding this comment

mejiaj Nov 8, 2022

Choose a reason for hiding this comment

mitchmoccia commented Aug 18, 2022

mitchmoccia commented Nov 3, 2022

mitchmoccia commented Nov 3, 2022

mejiaj left a comment • edited Loading

Choose a reason for hiding this comment

amyleadem Aug 18, 2022 •

edited

Loading

mejiaj left a comment •

edited

Loading