fix(UnderlineNav2): underline nav items' selected state can be managed by the app state & clean up #3361

broccolinisoup · 2023-06-05T06:18:07Z

Describe your changes here.

State management issue

UnderlineNav items selected state was only manageable internally which wasn't an intentional decision, it was a bug. Folks reported the issue where they need to manage the selected state by the app state or separate navigation controls.

Clean up

Also took this PR as an opportunity to refactor and clean up some of the very early stage implementation details. I posted "Note for reviewers" comments to explain some of the not so obvious changes.

Screenshots

Before

Gif description: Example UnderlineNav navigation items are displayed with an overflow "More" menu. There is also a list of link items that manage the same navigation on the page. What is tested here is that when the navigation is managed by external controls (i.e. the list of link items not the UnderlineNav items), whether or not the UnderlineNav items reflect the current page by their selected state. UnderlineNav items are clicked to navigate to different pages and their selected state works fine however if the external controls are used to navigate to other routes, UnderlineNav items doesn't reflect the current page.

After

Gif description: Example UnderlineNav navigation items are displayed with an overflow "More" menu. There is also a list of link items that manage the same navigation on the page. What is tested here is that when the navigation is managed by external controls (i.e. the list of link items not the UnderlineNav items), whether or not the UnderlineNav items reflect the current page by their selected state. UnderlineNav items are clicked to navigate to different pages and their selected state works fine and now with the external controls are used to navigate to other routes, UnderlineNav items does reflect the current page.

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

…rSelect

changeset-bot · 2023-06-05T06:18:10Z

🦋 Changeset detected

Latest commit: 96794b7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-06-05T06:20:58Z

size-limit report 📦

Path	Size
dist/browser.esm.js	102.36 KB (-0.38% 🔽)
dist/browser.umd.js	102.92 KB (-0.37% 🔽)

src/UnderlineNav2/UnderlineNav.tsx

broccolinisoup · 2023-07-10T00:11:06Z

src/UnderlineNav2/styles.ts

@@ -10,26 +9,16 @@ export const iconWrapStyles = {
  marginRight: 2,
 }

-const smallVariantLinkStyles = {


Note for reviewers: These variants were considered in the initial version of the component but it is decided otherwise at the end. Removing them to reduce the clutter and complexity on the component and styles.

broccolinisoup · 2023-07-10T00:12:43Z

src/UnderlineNav2/UnderlineNav.tsx

-  // cariant and align are currently not in used. Keeping here until some design explorations are finalized.
-  variant?: 'default' | 'small'
-  align?: 'right'


Note for reviewers: variant and align were considered in the initial version of the component but it is being decided not to include them at the end. Removing them to reduce the clutter and complexity on the component and styles.

broccolinisoup · 2023-07-10T00:13:45Z

src/UnderlineNav2/UnderlineNav.tsx

  /**
   * loading state for all counters. It displays loading animation for individual counters (UnderlineNav.Item) until all are resolved. It is needed to prevent multiple layout shift.
   */
  loadingCounters?: boolean
-  afterSelect?: (event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => void


Note for reviewers: afterSelect was considered in the initial version of the component but it is being decided not to include it at the end. Removing it to reduce the clutter and complexity on the component

broccolinisoup · 2023-07-10T00:14:47Z

src/UnderlineNav2/UnderlineNav.tsx

    >(null)

    const [iconsVisible, setIconsVisible] = useState<boolean>(true)

-    const afterSelectHandler = (event: React.MouseEvent<HTMLLIElement> | React.KeyboardEvent<HTMLLIElement>) => {


After removing the afterSelect, this is not needed.

langermank · 2023-07-13T02:09:53Z

src/UnderlineNav2/UnderlineNavItem.tsx

          as={Component}
          href={href}
-          onKeyPress={keyPressHandler}
-          onClick={clickHandler}
          aria-current={ariaCurrent}


I saw some conversation about aria-current. It is the preferred method for accessibility, but I think it can be hard to control sometimes (at least that has been our experience so far with NavList). But maybe we can continue using it but handle the state of it somehow with more control? Or offer multiple solutions?

but I think it can be hard to control sometimes (at least that has been our experience so far with NavList)

Thanks for sharing this! I personally have mix feelings about this API and felt validated when you shared the feedback 😅 We changed the prop name from selected to aria-current to align with NavList. But if we already have feedback from NavList, maybe it is a good time to chat about in the Primer API session??

…apper out of the UnderlineNavItem component

broccolinisoup · 2023-07-18T06:42:22Z

src/UnderlineNav2/UnderlineNav.tsx

-            {responsiveProps.items}
+            {listLinks.map(listLink => {
+              return (
+                <Box


I moved the Box wrapper here to make UnderlineNav return <Link></Link> instead of <Box><Link></Link></Box to align with as PolymorphicForwardRefComponent<'a', UnderlineNavItemProps> but let me know if you have any concern. I am not 100% sure about that.

joshblack

🥳

broccolinisoup · 2023-07-20T06:57:54Z

And no aria-current needed because it will be handled internally?

As a user of this component, I would prefer the defaultSelected prop on the UnderlineNav.Item. This is to me much easier to understand than the aria-current one, but I don't know the implication on accessibility nor coherence with the other primer components.

@echarles Thanks for the feedback! We are thinking to go ahead with this implementation - I'll address that in a separate PR!

echarles · 2023-07-20T08:00:31Z

@echarles Thanks for the feedback! We are thinking to go ahead with this implementation - I'll address that in a separate PR!

@broccolinisoup Sounds great. Now this is merged, I will give a try with the snapshots to confirm the state can be managed from the app.

echarles · 2023-07-21T05:07:55Z

@broccolinisoup Sounds great. Now this is merged, I will give a try with the snapshots to confirm the state can be managed from the app.

Just tried with "@primer/react": "36.0.0-rc.59fbc025" published a few hours ago (the next major snapshot), but it dos not seem to work. I have the same behavior as before: Setting from the app aria-current to page does not change the selected item.

broccolinisoup · 2023-07-21T06:52:03Z

Hi @echarles! The canary you are looking at it is the major release. The major change branch is behing a few commits from main. If you would like to test these changes, could you please try installing the canary version from the minor release branch? The last one seem to be 35.27.0-rc.7108ad52 Let me know how you go!

echarles · 2023-07-21T06:59:09Z

@broccolinisoup oh my bad. tbh I am confused with the major/minor snapshots. No worries, I will look in more details that point later.

Great news, 35.27.0-rc.7108ad52 works fine now for me. I can manage the nav items from the app. That was expected, but good to confirm.

broccolinisoup added 3 commits June 5, 2023 11:10

initial commit

9cee89f

second commit

000945b

make menu item pop out if an outside click is triggered & remove afte…

3f25597

…rSelect

broccolinisoup temporarily deployed to github-pages June 5, 2023 06:24 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3361 June 5, 2023 06:24 Inactive

broccolinisoup added 2 commits June 5, 2023 17:38

remove spa example

b535316

ts ignore comment

c5a6014

broccolinisoup temporarily deployed to github-pages June 5, 2023 07:45 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3361 June 5, 2023 07:46 Inactive

broccolinisoup added 2 commits June 5, 2023 18:04

take onMenuItemClick our of props

c1daa01

move menu item component back in underlinenav again

4b6a964

broccolinisoup temporarily deployed to github-pages June 5, 2023 08:23 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3361 June 5, 2023 08:23 Inactive

broccolinisoup mentioned this pull request Jun 5, 2023

UnderlineNav v2 item can enter a selected non-current state #2865

Closed

echarles reviewed Jun 22, 2023

View reviewed changes

src/UnderlineNav2/UnderlineNav.tsx Outdated Show resolved Hide resolved

Merge branch 'main' into bs/underline-nav-selected-state

d3b2374

broccolinisoup temporarily deployed to github-pages July 10, 2023 00:00 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3361 July 10, 2023 00:01 Inactive

broccolinisoup commented Jul 10, 2023

View reviewed changes

broccolinisoup added 2 commits July 10, 2023 14:01

fix the overflow styling and clean the code from unnecessary logics

517c4c7

add changeset

783cd19

broccolinisoup changed the title ~~Bs/underline nav selected state~~ fix(UnderlineNav2): underline nav items' selected state can be managed by the app state & clean up Jul 10, 2023

broccolinisoup temporarily deployed to github-pages July 10, 2023 04:11 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3361 July 10, 2023 04:11 Inactive

update snapshots

01e5817

langermank reviewed Jul 13, 2023

View reviewed changes

broccolinisoup added 2 commits July 18, 2023 16:27

remove unnecessary useEffect & address illegal state & take the li wr…

06a4edc

…apper out of the UnderlineNavItem component

changed components UnderlineNav

fa4ff65

github-actions bot temporarily deployed to storybook-preview-3361 July 18, 2023 06:34 Inactive

broccolinisoup temporarily deployed to github-pages July 18, 2023 06:35 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3361 July 18, 2023 06:36 Inactive

broccolinisoup commented Jul 18, 2023

View reviewed changes

joshblack approved these changes Jul 19, 2023

View reviewed changes

broccolinisoup added 2 commits July 20, 2023 16:32

clean up and add a test

662fb28

remove console error spy

96794b7

github-actions bot temporarily deployed to storybook-preview-3361 July 20, 2023 06:41 Inactive

broccolinisoup deployed to github-pages July 20, 2023 06:46 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-3361 July 20, 2023 06:46 View deployment

broccolinisoup added this pull request to the merge queue Jul 20, 2023

Merged via the queue into main with commit bea39c2 Jul 20, 2023
28 checks passed

broccolinisoup deleted the bs/underline-nav-selected-state branch July 20, 2023 07:07

primer-css mentioned this pull request Jul 20, 2023

Release tracking #3493

Merged

This was referenced Jul 25, 2023

UnderlineNav2: Take the listing html structure back to its original state #3559

Merged

Release Tracking #3532

Closed

primer-css mentioned this pull request Jul 27, 2023

Release tracking (next major) #3147

Closed

skemex mentioned this pull request May 24, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#45

Open

rbragaru mentioned this pull request Aug 14, 2024

[Snyk] Upgrade @primer/react from 35.15.1 to 35.32.2 rbragaru/rocketdex#4

Open

jaked mentioned this pull request Sep 3, 2024

UnderlineNav doesn't update its tabs when the items change #4914

Open

skemex mentioned this pull request Sep 24, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#55

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(UnderlineNav2): underline nav items' selected state can be managed by the app state & clean up #3361

fix(UnderlineNav2): underline nav items' selected state can be managed by the app state & clean up #3361

broccolinisoup commented Jun 5, 2023 •

edited

Loading

changeset-bot bot commented Jun 5, 2023 •

edited

Loading

github-actions bot commented Jun 5, 2023 •

edited

Loading

broccolinisoup Jul 10, 2023

broccolinisoup Jul 10, 2023

broccolinisoup Jul 10, 2023

broccolinisoup Jul 10, 2023

langermank Jul 13, 2023

broccolinisoup Jul 13, 2023

broccolinisoup Jul 18, 2023

joshblack left a comment

broccolinisoup commented Jul 20, 2023

echarles commented Jul 20, 2023

echarles commented Jul 21, 2023

broccolinisoup commented Jul 21, 2023

echarles commented Jul 21, 2023 •

edited

Loading

fix(UnderlineNav2): underline nav items' selected state can be managed by the app state & clean up #3361

fix(UnderlineNav2): underline nav items' selected state can be managed by the app state & clean up #3361

Conversation

broccolinisoup commented Jun 5, 2023 • edited Loading

State management issue

Clean up

Screenshots

Before

After

Merge checklist

changeset-bot bot commented Jun 5, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Jun 5, 2023 • edited Loading

size-limit report 📦

broccolinisoup Jul 10, 2023

Choose a reason for hiding this comment

broccolinisoup Jul 10, 2023

Choose a reason for hiding this comment

broccolinisoup Jul 10, 2023

Choose a reason for hiding this comment

broccolinisoup Jul 10, 2023

Choose a reason for hiding this comment

langermank Jul 13, 2023

Choose a reason for hiding this comment

broccolinisoup Jul 13, 2023

Choose a reason for hiding this comment

broccolinisoup Jul 18, 2023

Choose a reason for hiding this comment

joshblack left a comment

Choose a reason for hiding this comment

broccolinisoup commented Jul 20, 2023

echarles commented Jul 20, 2023

echarles commented Jul 21, 2023

broccolinisoup commented Jul 21, 2023

echarles commented Jul 21, 2023 • edited Loading

broccolinisoup commented Jun 5, 2023 •

edited

Loading

changeset-bot bot commented Jun 5, 2023 •

edited

Loading

github-actions bot commented Jun 5, 2023 •

edited

Loading

echarles commented Jul 21, 2023 •

edited

Loading