-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Nav): Updated to add wrapper for nav link text #9740
Changes from 5 commits
1dd2378
e19086a
9e6f078
0c8dd70
02a5b28
26ec5c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,8 @@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, ' | |
ouiaId?: number | string; | ||
/** Set the value of data-ouia-safe. Only set to true when the component is in a static state, i.e. no animations are occurring. At all other times, this value must be false. */ | ||
ouiaSafe?: boolean; | ||
/** @beta Adds a wrapper around the nav link text. Improves the layout when the text is a react node. */ | ||
hasNavLinkWrapper?: boolean; | ||
} | ||
|
||
export const NavItem: React.FunctionComponent<NavItemProps> = ({ | ||
|
@@ -56,6 +58,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({ | |
ouiaId, | ||
ouiaSafe, | ||
zIndex = 9999, | ||
hasNavLinkWrapper, | ||
...props | ||
}: NavItemProps) => { | ||
const { flyoutRef, setFlyoutRef, navRef } = React.useContext(NavContext); | ||
|
@@ -188,7 +191,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({ | |
{...(hasFlyout && { ...ariaFlyoutProps })} | ||
{...props} | ||
> | ||
{children} | ||
{hasNavLinkWrapper ? <span className={css(`${styles.nav}__link-text`)}>{children}</span> : children} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
{flyout && flyoutButton} | ||
</Component> | ||
); | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,6 +7,9 @@ ouia: true | |||||
--- | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
|
||||||
## Examples | ||||||
|
||||||
|
@@ -68,6 +71,12 @@ A flyout should be a `Menu` component. Press `space` or `right arrow` to open a | |||||
|
||||||
``` | ||||||
|
||||||
### Link text | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know that the use case is specifically for an icon. You can pass any react node. |
||||||
|
||||||
```ts isBeta file="./NavLinkText.tsx" | ||||||
|
||||||
``` | ||||||
|
||||||
## Types | ||||||
|
||||||
### NavSelectClickHandler | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,107 @@ | ||||||
import React from 'react'; | ||||||
import { Nav, NavExpandable, NavItem, NavList } from '@patternfly/react-core'; | ||||||
import ArrowRightIcon from '@patternfly/react-icons/dist/esm/icons/arrow-right-icon'; | ||||||
import UserIcon from '@patternfly/react-icons/dist/esm/icons/user-icon'; | ||||||
|
||||||
export const NavLinkText: React.FunctionComponent = () => { | ||||||
const [activeGroup, setActiveGroup] = React.useState('nav-expand3rd-group-1'); | ||||||
const [activeItem, setActiveItem] = React.useState('nav-expand3rd-group-1_item-1'); | ||||||
|
||||||
const onSelect = ( | ||||||
_event: React.FormEvent<HTMLInputElement>, | ||||||
result: { itemId: number | string; groupId: number | string } | ||||||
) => { | ||||||
setActiveGroup(result.groupId as string); | ||||||
setActiveItem(result.itemId as string); | ||||||
}; | ||||||
|
||||||
const onToggle = ( | ||||||
_event: React.MouseEvent<HTMLButtonElement>, | ||||||
result: { groupId: number | string; isExpanded: boolean } | ||||||
) => { | ||||||
// eslint-disable-next-line no-console | ||||||
console.log(`Group ${result.groupId} expanded? ${result.isExpanded}`); | ||||||
}; | ||||||
|
||||||
return ( | ||||||
<Nav onSelect={onSelect} onToggle={onToggle} aria-label="Nav link text"> | ||||||
<NavList> | ||||||
<NavItem | ||||||
preventDefault | ||||||
id="link-text-link-1" | ||||||
to="#link-text-link-1" | ||||||
itemId="link-text-1-item-1" | ||||||
isActive={activeItem === 'link-text-1-item-1'} | ||||||
hasNavLinkWrapper | ||||||
> | ||||||
Link 1 | ||||||
<ArrowRightIcon /> | ||||||
</NavItem> | ||||||
<NavExpandable | ||||||
title={ | ||||||
<> | ||||||
Link 2<small>(small text)</small> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a space
Suggested change
|
||||||
</> | ||||||
} | ||||||
groupId="link-text-group-1" | ||||||
isActive={activeGroup === 'link-text-group-1'} | ||||||
isExpanded | ||||||
> | ||||||
<NavItem | ||||||
preventDefault | ||||||
id="link-text-link-2" | ||||||
to="#link-text-link-2" | ||||||
groupId="link-text-group-1" | ||||||
itemId="link-text-group-1_item-1" | ||||||
isActive={activeItem === 'link-text-group-1_item-1'} | ||||||
> | ||||||
<UserIcon /> | ||||||
Subnav link 1 | ||||||
Comment on lines
+59
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
</NavItem> | ||||||
<NavItem | ||||||
preventDefault | ||||||
id="link-text-link-3" | ||||||
to="#link-text-link-3" | ||||||
groupId="link-text-group-1" | ||||||
itemId="link-text-group-1_item-2" | ||||||
isActive={activeItem === 'link-text-group-1_item-2'} | ||||||
> | ||||||
<UserIcon /> | ||||||
Subnav link 2 | ||||||
</NavItem> | ||||||
</NavExpandable> | ||||||
<NavExpandable | ||||||
title={ | ||||||
<> | ||||||
Link 3<strong>(strong text)</strong> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
</> | ||||||
} | ||||||
groupId="link-text-group-2" | ||||||
isActive={activeGroup === 'nav-expand3rd-group-2'} | ||||||
isExpanded | ||||||
> | ||||||
<NavItem | ||||||
preventDefault | ||||||
id="link-text-link-4" | ||||||
to="link-text-link-4" | ||||||
groupId="link-text-group-2" | ||||||
itemId="link-text-group-2_item-1" | ||||||
isActive={activeItem === 'link-text-group-2_item-1'} | ||||||
> | ||||||
Subnav link 1 | ||||||
</NavItem> | ||||||
<NavItem | ||||||
preventDefault | ||||||
id="link-text-link-5" | ||||||
to="link-text-link-5" | ||||||
groupId="link-text-group-2" | ||||||
itemId="link-text-group-2_item-2" | ||||||
isActive={activeItem === 'link-text-group-2_item-2'} | ||||||
> | ||||||
Subnav link 2 | ||||||
</NavItem> | ||||||
</NavExpandable> | ||||||
</NavList> | ||||||
</Nav> | ||||||
); | ||||||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we open an issue to make this the default in the next breaking change, if there isn't one already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcoker breaking change issue opened #9755