fix(Nav): updated drilldown navigation heights #9255
Conversation
|
Preview: https://patternfly-react-pr-9255.surge.sh A11y report: https://patternfly-react-pr-9255-a11y.surge.sh |
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
The component example is working, but I still see a problem in the demo here: https://patternfly-react-pr-9255.surge.sh/components/navigation/react-demos/drilldown-nav/
|
@mcarrano good catch, should now be working as well |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Fixes LGTM.
Only question I have is that on the demo the top nav item in a drilled in menu is permanently highlighted, which I understand because its the active submenu (or should it not highlight?). But the hover highlight is the same color so it looks a bit weird trying to select another item.
Good point. It does match the behavior seen in our Menu drilldown example, and having it highlight also can help convey what item in the new menu has focus for users as well. But yeah the style matching the hover style can be confusing. @mcarrano what do you think? |
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
LGTM - pending @mcarrano's opinion about styles
I have to admit this looks a bit off. If I compare it to this demo, for example, the hover and selected styles are slightly different. This may have been an oversight with the original implementation, and unless the styling changed for v5 or there is a simple fix, I say merge this and perhaps open a follow-up design issue. @mcoker @mceledonia can you take a quick look and see if this styling matches your expectation? It's this demo: https://patternfly-react-pr-9255.surge.sh/components/navigation/react-demos/drilldown-nav |
|
Yeah, I don't think the background change is necessary here, it seems pretty clear you're in a new navigation context without it. |
|
@mcarrano if you are ok with it, let's open a follow up issue. |
|
@tlabaj yes, I am OK with that. I can approve this and open a new design issue. |
|
Opened this follow-up issue: patternfly/patternfly-design#1263 |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
* fix(Nav): updated drilldown navigation heights * Bump reactcore version * Updated drilldown demo
7 participants
What: Closes #9183
Additional issues: