feat: Added Tab updates for site Navigation #12111
Conversation
|
Preview: https://pf-react-pr-12111.surge.sh A11y report: https://pf-react-pr-12111-a11y.surge.sh |
There was a problem hiding this comment.
File comment/question below for Michael, and also @mcoker are we planning on adding high contrast styles for the pf-m-nav class? Currently any HC styles seem to be lost when that's applied.
We'll also need some tests as part of this, though that may depend on the resolution to my comment below for Michael.
|
|
||
| const uniqueId = id || getUniqueId(); | ||
| const Component: any = component === TabsComponent.nav ? 'nav' : 'div'; | ||
| const Component: any = component === TabsComponent.nav || isNav ? 'nav' : 'div'; |
There was a problem hiding this comment.
@mcoker to get this to work as discussed - where you can override the wrapper element when isNav is true by passing component="div" - we'd have to remove the default value of the component prop from the defaultProps and replace it around here. This is because component is defaulted to a div, and if we say "make this element a nav when isNav is true, except when the component is passed div as a value" then that just happens by default so a consumer would also have to pass component="nav"
Technically everything should remain the same - Tabs will default to a div wrapper as they currently do on Org examples - main thing is that the props table would no longer show the div being the default value in the table cell.
If we still want to allow isNav to dictate the nav modifier class being applied AND the wrapper element defaulting to a nav element, what I did locally was (all in Tabs.tsx file):
- Remove the default component value on line ~209
- throww
const defaultComponent = isNav && !component ? 'nav' : 'div';above theconst Componentline on ~532 - update the logic for the compnent to
component === TabsComponent.nav || (isNav && component !== 'div') ? 'nav' : defaultComponent
(all this could be cleaned up I'm sure)
There was a problem hiding this comment.
+1 to let component override the wrapper even when isNav is passed in. But can be done in a follow up if the above solution ends up complicated.
packages/react-core/src/components/Tabs/examples/TabsSiteNav.tsx
Outdated
Show resolved
Hide resolved
| const uniqueId = id || getUniqueId(); | ||
| const Component: any = component === TabsComponent.nav ? 'nav' : 'div'; | ||
| const defaultComponent = isNav && !component ? 'nav' : 'div'; | ||
| const Component: any = component === TabsComponent.nav || (isNav && component !== 'div') ? 'nav' : defaultComponent; |
There was a problem hiding this comment.
532's logic is a little confusing to me, would this be equivalent and slightly cleaner?
const Component: any = component !== undefined ? component : defaultComponent;
Because defaultComponent is already set to "nav" for isNav and "div" otherwise, and if TabsComponent.nav is passed in that's should already be setting it to "nav".
There was a problem hiding this comment.
Yep that's the part re "this could be cleaned up Im sure" 😆 Using the logic you mention does work the same so using that would be better
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
Resolves issue #12080