feat(Nav): Updated to add wrapper for nav link text#9740
feat(Nav): Updated to add wrapper for nav link text#9740kmcfaul merged 6 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-react-pr-9740.surge.sh A11y report: https://patternfly-react-pr-9740-a11y.surge.sh |
thatblindgeye
left a comment
There was a problem hiding this comment.
The updates from before the latest commit looked good I believe. Currently the NavItem doesn't have the new nav__link-text class being rendered like it should compared to Core; that new class is only being rendered for the NavExpandable component.
The non-expandable NavItem in Core:
The non-expandable NavItem in this PR:
|
|
||
| ``` | ||
|
|
||
| ### Link text |
There was a problem hiding this comment.
Nit: I feel like the example name could be tweaked a little, something like "With text and icon" or "Custom link text" or something. Not sure if "link text" fully conveys what the example is showing
There was a problem hiding this comment.
I don't know that the use case is specifically for an icon. You can pass any react node.
| {...buttonProps} | ||
| > | ||
| {title} | ||
| {typeof title !== 'string' ? <span className={css(`${styles.nav}__link-text`)}>{title}</span> : title} |
There was a problem hiding this comment.
lgtm, only other thing is that we're having this wrapper be an opt-in for the NavItem, but providing logic here to determine if the wrapper is used or not. It seems like we'd want it to be opt in in both places?
There was a problem hiding this comment.
So it is not opt in here because we did not have the ability to pass a react node before. Menaing. that it is a new feature. We will not be breaking anyone.
There was a problem hiding this comment.
Can we open an issue to make this the default in the next breaking change, if there isn't one already?
| {...props} | ||
| > | ||
| {children} | ||
| {hasNavLinkWrapper ? <span className={css(`${styles.nav}__link-text`)}>{children}</span> : children} |
mcoker
left a comment
There was a problem hiding this comment.
LGTM! I have questions about the example, but I think the feature is good-to-go 🚀
|
|
||
| import './nav.css'; | ||
| import ArrowRightIcon from '@patternfly/react-icons/dist/esm/icons/arrow-right-icon'; | ||
| import UserIcon from '@patternfly/react-icons/dist/esm/icons/user-icon'; |
There was a problem hiding this comment.
| import UserIcon from '@patternfly/react-icons/dist/esm/icons/user-icon'; | |
| import UserIcon from '@patternfly/react-icons/dist/esm/icons/user-icon'; |
| <NavExpandable | ||
| title={ | ||
| <> | ||
| Link 2<small>(small text)</small> |
There was a problem hiding this comment.
This needs a space
| Link 2<small>(small text)</small> | |
| Link 2 <small>(small text)</small> |
| <NavExpandable | ||
| title={ | ||
| <> | ||
| Link 3<strong>(strong text)</strong> |
There was a problem hiding this comment.
| Link 3<strong>(strong text)</strong> | |
| Link 3 <strong>(strong text)</strong> |
| <UserIcon /> | ||
| Subnav link 1 |
There was a problem hiding this comment.
Did you mean to add hasNavLinkWrapper to this and the one below? With adding the space to the 2 items I left comments about, and adding hasNavLinkWrapper here and the next item, it would look like this:
Here's what it looks like now:
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #9735