bug(Nav): Fixed scroll buttons always showing

[tlabaj]

trafficstars

What: Closes #9977

pf-m-scrollable is no longer hiding the buttons. Updated React to hide them instead.

[patternfly-build]

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

A11y report: https://patternfly-react-pr-10070-a11y.surge.sh

[andrew-ronaldson]

Just a couple of questions:

• Looks like this update will also remove a scroll button once you've scrolled to the end of the nav list and there is nothing left to scroll? If so, I wonder if it's a better pattern to have that button change to disabled once you reach the end. Otherwise you may click a scroll button repeatedly to get to one side of the nav item list, and when you reach the end and the button disappears, you may end up accidentally clicking on a nav link. wdyt @lboehling @andrew-ronaldson

  • Also from the figma, we have a "disabled" style for those scroll buttons and AFAIK reaching the end of the list would be the only use case for a disabled scroll button? So that seems to imply they should be disabled instead of disappearing.

• Is this up to date with the latest from core v6? I'm noticing some styling issues.

I agree with the disabled state rather than removing the scroll button entirely.

[tlabaj]

@mcoker is Their a disabled modifier/class for the button? I do not see it in the css.

[mcoker]

@tlabaj added a core PR here - https://github.com/patternfly/patternfly/pull/6302/

All that should be needed is to use the regular method of disabling the <Button> that's used there, though it's worth noting that this button was using the button-icon wrapper for the icon, and that's not needed for plain buttons and results in incorrect styling. I removed that in the core examples, but looks like it was added to the react component, too, so you'll need to take this out as well and just include the icon as the child of <Button> like any other plain button.

https://github.com/patternfly/patternfly-react/blob/7205b29995cc7d458b96724eb13e6aef2ccb2f81/packages/react-core/src/components/Nav/NavList.tsx#L155

[mcoker]

Nav changes LGTM!

Though I made some modifications locally to the examples to make sure they look correct. Looks like we probably need to update the masthead code use across the react examples/demos a little, though shouldn't block this PR.

• These demos have isFullHeight on the toolbar in the masthead. Looks like we've removed that in core, so I'm assuming it's safe to remove in react, too.
• The user menu with the avatar is currently making the masthead/toolbar taller than it needs to be resulting the toolbar items being misaligned. I don't see that menu in the core examples/demos anymore (it's just the ellipsis icon) though I do see the image below in figma, so I'm not sure what the ideal presentation is there.
  • 

@mattnolting worked on the masthead and toolbar - can you verify we're no longer using isFullHeight on the toolbar in the masthead? And do you know what we're doing with the user menu with the avatar? cc @lboehling @andrew-ronaldson

[mattnolting]

@mattnolting worked on the masthead and toolbar - can you verify we're no longer using isFullHeight on the toolbar in the masthead? And do you know what we're doing with the user menu with the avatar? cc @lboehling @andrew-ronaldson

The spec no longer includes a full height version of the masthead as the interactive elements within have no full height variant. Can you confirm @lboehling @andrew-ronaldson?

In regard to the avatar, applying the pf-m-sm modifier will resolve the issue.

[patternfly-build]

Your changes have been released in:

• @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]

Thanks for your contribution! :tada: