chore(Nav): Nav Drilldown example & demo

[jpuzz0]

What: Closes #6299

[patternfly-build]

Preview: https://patternfly-react-pr-6875.surge.sh

[mcarrano]

@jeffpuzzo this is looking good, but I did have a few comments/questions.

• I'm not sure why the header (back) link is displaying as selected. When you drill into a new level, the active page should show as selected.

• I'm also not sure why I am seeing a scrollbar on the nav even though there is plenty of space for the menu to live.

• Lastly I'm seeing some styling differences between the back/header in the example and the design spec/prototype. Here's what the original spec looked like:

You can see where the header text is slightly heavier and the bottom border is thicker. I realize that these may be core styling issues that were missed in the original review, and therefore am copying in @mattnolting to comment. @mceledonia would also be good to get your input since (Becca) the original designer is no longer with the company.

[mattnolting]

@mcarrano Looking back at drilldown development (2365), the chevron is positioned as presented in the design. Back button font is heavier and divider is thicker.

I navigational context, the design moves the back chevron to the left slightly.

These are probably an oversight and should be updated in Core.

Opened a new issue: patternfly/patternfly#4656

[mcarrano]

Thanks @mattnolting !

[mmenestr]

Outside of what @mcarrano mentioned, I noticed that when you click on a drill down item, the top "header item" with the back arrow that tells you which "drill down" you're in takes a few seconds to become highlighted. Is that lag something that can be changed so that it's more instantaneous?

Screen.Recording.2022-02-02.at.6.06.33.PM.mov

[mattnolting]

Implementation looks solid to me, great job Jeff!

Side note, looks like we need to do some cleanup work on the menu component as noted in #6881, #4656, 4655

[nicolethoen]

@mattnolting @mcarrano
I did note that the drilldown menu does have the extraneous scroll bar in this issue: #6842. It is not new with this implementation

[nicolethoen]

• I'm not sure why the header (back) link is displaying as selected. When you drill into a new level, the active page should show as selected.

This is how the current drilldown menu works as well. By drilling in, they are not navigating to new pages and the header is basically highlighted because it's the last item the the user has interacted with. When they click on an item that does not have a submenu, it will navigate to that page and highlight the correct item.

[mcarrano]

By drilling in, they are not navigating to new pages and the header is basically highlighted because it's the last item the the user has interacted with. When they click on an item that does not have a submenu, it will navigate to that page and highlight the correct item.

@nicolethoen Thanks for that explanation. I get what you are saying, but I'm not sure if that's what we should expect for this in the context of navigation. My expectation was that for a drill down navigation, the first page in a new section would be automatically selected when I drill in. This is different from expandable or fly-out navs where clicking into a new level exposes new nav items but does not navigate to a new page. I'd need to do more testing to validate my assumptions here and would also be interested to hear expectations from @mmenestr and @mceledonia .

[kmcfaul]

Implementation lgtm.

I personally would not expect drilling in or out to actually navigate to a new page, but that behavior should be able to be added in on the demo side if a user desires, inside the drill in/out handlers.

[mmenestr]

@mcarrano I kind of see it as Nicole described it, which is why the fact that the header was selected didn't stand out to me as weird. Now that you mention it though, I'm not sure it's necessary for the header to be highlighted either way. And then in terms of your thought that "the first page in a new section would be automatically selected when I drill in", I don't think I expected that either actually!

If you compare it to a regular expandable navigation item, when you expand it, (1) The expanded item doesn't get highlighted, the caret just changes direction, and (2) you don't get navigated to the first page within the expansion automatically - the user has to make a page selection themselves, after the expansion.

Not sure if it's fair to make a direct comparison, but I just see this as an expandable navigation item that's just laid out differently, and if that was the intention, then I think it makes sense to follow the same interaction patterns!

[mcarrano]

Thanks for weighing in on this @mmenestr. Seems hard to know what the right answer, so maybe we just keep this behavior for now. It is marked as beta so I think we can adjust once we learn more about intended use. I'm not aware that we have a specific product use case at present.

@jeffpuzzo @nicolethoen I do see that this is following the drill-down pattern established here: https://www.patternfly.org/v4/components/menu#with-drilldown If a consumer wanted to modify this to force selection to the first menu item, any reason they couldn't do that? To @mmenestr 's question, must we always have a selected item?

I do notice that we have some inconsistency between https://www.patternfly.org/v4/components/menu#with-drilldown and https://www.patternfly.org/v4/components/menu#with-drilldown-breadcrumbs however. The later works more like I was expecting.

[thatblindgeye]

Agreed on implementation looking good!

In regards to the conversation about the behavior of that back/header item being selected vs similar other menus/navs, the way I had viewed it is that it makes sense in this instance for that back/header item to be selected after drilling in since it's the first item in that new list. I think this behavior being different for similar menus/navs makes sense in those instances since visually the entire menu/nav is still visible I believe, whereas here once you drill in, you lose visual access to the previous list of items.

That said I also understand this behavior being jarring compared to similar menus/navs.

[jpuzz0]

@mcarrano If by the "first menu item" you mean the menu item just below the one used to navigate back to the previous menu, then no, it seems that the focus logic there is, find the first list item and focus on it and it isn't controlled by a prop.

patternfly-react/packages/react-core/src/components/Menu/Menu.tsx

Lines 151 to 154 in 8c1f12c

	const nextTarget = Array.from(nextMenu.getElementsByTagName('UL')[0].children).filter(
	el => !(el.classList.contains('pf-m-disabled') || el.classList.contains('pf-c-divider'))
	)[0].firstChild;
	(nextTarget as HTMLElement).focus();

To answer your other question of if we even need a selected item, from my perspective, I don't think it's necessary to auto-focus on the first item in the list when drilling down.

My follow-up questions:

1. When do we want focus to happen (if at all)? @jessiehuff - I've been told you might have some insight for this.
2. If we want focus to happen, do we want it to be controllable by consumers as to which gets focused (since it isn't right now)?

[jpuzz0]

Outside of what @mcarrano mentioned, I noticed that when you click on a drill down item, the top "header item" with the back arrow that tells you which "drill down" you're in takes a few seconds to become highlighted. Is that lag something that can be changed so that it's more instantaneous?

Screen.Recording.2022-02-02.at.6.06.33.PM.mov

@mmenestr I'm not a fan of the delay either. From what I can tell, the way it's implemented is resulting in the slight delay (how we're querying the list items and determining which one to select). We will have to optimize the performance of this component to remedy the problem IMO. Is this something we might want to open an issue for @tlabaj @nicolethoen?

[tlabaj]

@jeffpuzzo yes. We can ope a follow up issue for the performance.

[jessiehuff]

When do we want focus to happen (if at all)? @jessiehuff - I've been told you might have some insight for this.
If we want focus to happen, do we want it to be controllable by consumers as to which gets focused (since it isn't right now)?

I'm not completely positive I understand the question, though admittedly there's a lot of conversation to catch up on here. Are we asking where focus should move when selecting a menu item? The way it currently is makes sense to me at the moment, as I can imagine drilling into a menu to preview items and going back. I don't see a massive issue with having an initial selection either though similar to our dropdown with initial selection. Otherwise I'd expect focus to behave in the same way as other components (up/down arrow keys within a menu), enter to select buttons, Esc to close the menu (though I suppose that one is on the consumer in this situation). I did notice through testing that it has an odd behavior in VO where you are forced to drill all the way into all of the first menu's submenus and are never able to go to the second option in the initial menu or any of its submenus. However, it looks like that's also an issue in our drilldown menu example.

[jpuzz0]

When do we want focus to happen (if at all)? @jessiehuff - I've been told you might have some insight for this.
If we want focus to happen, do we want it to be controllable by consumers as to which gets focused (since it isn't right now)?

I'm not completely positive I understand the question, though admittedly there's a lot of conversation to catch up on here. Are we asking where focus should move when selecting a menu item? The way it currently is makes sense to me at the moment, as I can imagine drilling into a menu to preview items and going back. I don't see a massive issue with having an initial selection either though similar to our dropdown with initial selection. Otherwise I'd expect focus to behave in the same way as other components (up/down arrow keys within a menu), enter to select buttons, Esc to close the menu (though I suppose that one is on the consumer in this situation). I did notice through testing that it has an odd behavior in VO where you are forced to drill all the way into all of the first menu's submenus and are never able to go to the second option in the initial menu or any of its submenus. However, it looks like that's also an issue in our drilldown menu example.

@jessiehuff Thanks for the reply. What I've gathered from discussions here is that some folks are asking if it makes sense to keep the "breadcrumb" link focused initially when drilling down into menus (where the logic is just to select the first indexed li), then the question of whether we need the drilldown menu to auto-focus at all came up. Then there's the possibility of this being controlled by the component, where maybe you can specify an id or index (some unique identifier is probably preferred) to allow consumers control over what is focused instead of us doing it for them.

Would you say you're ok with focus being controllable by a prop and/or removing the auto-focus we have on the first li in the drilldown menus?

[tlabaj]

We can take this PR in as is since it is Beta, we can revisit and make modifications as needed.

[patternfly-build]

Your changes have been released in:

• [email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• [email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• [email protected]

Thanks for your contribution! 🎉