Skip to content
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

fix(react): uishell sidenavmenu active descendants #15027

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

cordesmj
Copy link
Contributor

Changelog

Changed

  • Updated the React SideNavMenu in UIShell so that it detects active descendant links when more than two levels of navigation are in the page heirarchy

Testing / Reviewing

  • Create HeaderNavigation with three levels of nesting.
  • Set a third-level link to 'isActive'
  • Shrink the window so that the hamburger menu is displayed.
  • Notice that the top level link in the side navigation is marked as active when the sub-link menu is not expanded.

Related to #14942

@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 8e0221f
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/65398dd82ec41800085e1a6d
😎 Deploy Preview https://deploy-preview-15027--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 8e0221f
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/65398dd800ceab0008249e5c
😎 Deploy Preview https://deploy-preview-15027--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 4f08770
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/653abe9d805e2b00086c2587
😎 Deploy Preview https://deploy-preview-15027--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 4f08770
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/653abe9d9ac0a30008daa1b2
😎 Deploy Preview https://deploy-preview-15027--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

when more than two levels of navigation are in the page heirarchy

Unfortunately this is out of spec for this component.

The left panel does not support three tiers of navigation. If you have additional content to display beneath a sub-menu, use tabs within the page.

@laurenmrice could you confirm this is still an intentional limitation of the UI Shell left side nav? I know we've had some variations of this for the platform work, etc. but I think in those instances we used TreeView? I recall @alisonjoseph mentioning this at one point.

@kingtraceyj
Copy link
Member

@cordesmj similar to my comment on the other PR, can we learn a little more about your need for the 3rd level? we have had some teams ask for this and i think it's a more common pattern to have a 3rd level on the side navigation than the header nav. the only thing is that we've seen it implemented as a tree view rather than a navigational component. this is done so users are able to navigate with the keyboard easier. i'm wondering if something like that would be a good fit?

@cordesmj
Copy link
Contributor Author

cordesmj commented Oct 27, 2023

@cordesmj similar to my comment on the other PR, can we learn a little more about your need for the 3rd level? we have had some teams ask for this and i think it's a more common pattern to have a 3rd level on the side navigation than the header nav. the only thing is that we've seen it implemented as a tree view rather than a navigational component. this is done so users are able to navigate with the keyboard easier. i'm wondering if something like that would be a good fit?

Thanks for you response @kingtraceyj. So I'm not sure how familiar you may be with w3 publisher. It is basically a webapp that allows people within IBM to create their own internal websites. We are currently in the process of converting it to carbon.

w3 Publisher has historically permitted users to create sites with up to three levels in the page hierarchy, and also, we allow users to choose between top-nav (header nav) or side nav, depending on their preferences or needs. We currently have tens of thousands of live sites, many of which use 3 levels. In support of the transition to carbon, our design team has designed extensions to carbons navigation elements to accomodate 3 levels in header nav, similar to:

image

and side nav, similar to:

image

These components have already undergone development, and have taken into account keyboard accessibility. The one hole that remains is when there is an active page highlighted. Similar to the implementation in carbon, when a parent has an active child, the parent is marked when the dropdown is not open. Unfortunately, marking the top level link when a third level nav element is active is not supported by carbon, which is what caused me to create these PR's, to add such support.

I am not sure if I am getting to the heart of your question, and being a developer and not a designer, I can't really speak to the design choice of tree view vs navigational component... I am not sure I even understand the implications of the distinction, beyond the fact that the TreeView component already exists.

If there is anything I could elaborate on to clarify things, or if it would help to tie in one of our design team, let me know.

@kingtraceyj
Copy link
Member

@cordesmj @tay1orjones after further discussion with the team, we don't want to be a blocker for your team to implement your requirements. for side navigation, teams have asked for the third level for sometime and we ourselves have done design explorations around this. our specs can be found in figma.

we are a bit more cautious about opening up a third level for header navigation—it can begin to complicate the information architecture structure pretty quickly and hide information. but, again, we don't want to block you from delivering your specific requirements so we can push this through as well.

thanks for contributing!

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Thanks @kingtraceyj!

@cordesmj
Copy link
Contributor Author

cordesmj commented Nov 2, 2023

Thanks @kingtraceyj!

Thanks for approving, @tay1orjones - can you also review #14942 please?

@tw15egan tw15egan added this pull request to the merge queue Nov 2, 2023
Merged via the queue into carbon-design-system:main with commit 72c2a05 Nov 2, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants