-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigation Component: Hide navigation item if target menu is empty #25746
Navigation Component: Hide navigation item if target menu is empty #25746
Conversation
Size Change: +263 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
packages/components/src/navigation/use-create-navigation-tree.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see how this feature might be useful and can reduce clutter in the navigation sidebar.
The more I think about it, however, the more I see how hiding navigation items might confuse the user instead. Using the Pages item as an example, clicking on it should lead to the TemplatesPagesMenu
. If we programmatically hide the menu, the user may be unaware that they have no Template Pages. They may be curious why the Pages
item suddenly disappeared.
The alternative, rather than restricting navigation to the TemplatesPagesMenu
, might be to show the menu with some generic empty state text. ("There are no Pages").
I'd love to hear your thoughts on this.
@jeyip We don't have a use-case for this feature right now. The design I followed (#23939 (comment)) mentions we should hide empty section. But we ditched it and we are showing all the menus all the time, that's what we agreed on with Jacopo. See the last comment in the linked issue, Jacopo threw up a ball regarding this topic. We don't know whether we need this feature yet. |
Ahah thanks @david-szabo97 I've been looking for that comment for hours now 😅 The tl;dr would be: I agree with you @jeyip, but it's mostly a design decision. Hiding empty items would unintentionally obfuscate the hierarchy, but on the other hand showing empty items would unnecessarily clutter the sidebar. |
Thanks for the link 🙏 Catching up on the conversations.
Gotcha 👍 |
I think we can start working on this again, maybe after mergin #25315, which adds more visibility info in context. 🤔 |
a11e356
to
cdc443d
Compare
Ready for review again 😄 |
packages/components/src/navigation/use-create-navigation-tree.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me 🚀
77e2072
to
2bc91ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really out of my depth with the proposed algorithm, but this works as advertised.
Given the use of the isEmpty
prop, and the fact that there won't realistically be tons of heavily nested menus, I wouldn't expect any significant performance hits.
651263b
to
fc48c5e
Compare
Description
The need for a solution like this came up in this PR: #25739
We need a solution to mark
NavigationMenu
as empty and avoid rendering all theNavigationItem
which are supposed to navigate to the empty menu. This is pretty straightforward for 1 levelNavigation
, but we can nestNavigationMenu
s which makes the a lot more interesting.This implementation traverses our
navigationTree
and goes through all the nested menus to check if they are empty. Empty is based on theisEmpty
prop which should be specified by the consumer onNavigationMenu
component. A menu is not considered empty if any nested menus areisEmpty={ false }
.This implementation exposes two new functions in the
navigationTree
context:traverseMenu
This function takes two parameters, first is the menu we should start traversing from. Second parameter is a callback which will be called for the menu specified in the first parameter and all the menus that are nested.
isMenuEmpty
This function takes one parameter, the menu we would like to check if it is empty or not. This function uses
traverseMenu
to go traverse the menu specified in the first parameter. If the specified menu is empty and all nested nested menus are empty as well, then this function returnstrue
. Otherwise it returnsfalse
NavigationItem
has a new boolean prop: hideIfTargetMenuEmpty. IfhideIfTargetMenuEmpty
istrue
, then it checks thenavigateToMenu
menu for emptiness. If the menu is empty, then thisNavigationItem
won't be rendered.How has this been tested?
yarn storybook:dev
Navigation
->Hide If Empty
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
Fixes #26265