fix(UnderlineNav2): underline nav items' selected state can be managed by the app state & clean up #3361
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@primer/react': patch | ||
| --- | ||
|
|
||
| UnderlineNav2: underline nav items' selected state can be managed by the app state | ||
|
|
||
| <!-- Changed components: UnderlineNav --> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,34 +3,31 @@ import Box from '../Box' | |
| import sx, {merge, BetterSystemStyleObject, SxProp} from '../sx' | ||
| import {UnderlineNavContext} from './UnderlineNavContext' | ||
| import {useResizeObserver, ResizeObserverEntry} from '../hooks/useResizeObserver' | ||
| import {useTheme} from '../ThemeProvider' | ||
| import {ChildWidthArray, ResponsiveProps, ChildSize} from './types' | ||
| import VisuallyHidden from '../_VisuallyHidden' | ||
| import {moreBtnStyles, getDividerStyle, getNavStyles, ulStyles, menuStyles, menuItemStyles, GAP} from './styles' | ||
| import styled from 'styled-components' | ||
| import {Button} from '../Button' | ||
| import {TriangleDownIcon} from '@primer/octicons-react' | ||
| import {useOnEscapePress} from '../hooks/useOnEscapePress' | ||
| import {useOnOutsideClick} from '../hooks/useOnOutsideClick' | ||
| import {useId} from '../hooks/useId' | ||
| import {ActionList} from '../ActionList' | ||
| import {defaultSxProp} from '../utils/defaultSxProp' | ||
| import CounterLabel from '../CounterLabel' | ||
| import {LoadingCounter} from './LoadingCounter' | ||
| import {invariant} from '../utils/invariant' | ||
|
|
||
| export type UnderlineNavProps = { | ||
| children: React.ReactNode | ||
| 'aria-label'?: React.AriaAttributes['aria-label'] | ||
| as?: React.ElementType | ||
| sx?: SxProp['sx'] | ||
| /** | ||
| * loading state for all counters. It displays loading animation for individual counters (UnderlineNav.Item) until all are resolved. It is needed to prevent multiple layout shift. | ||
| */ | ||
| loadingCounters?: boolean | ||
|
There was a problem hiding this comment. Note for reviewers: |
||
| } | ||
| // When page is loaded, we don't have ref for the more button as it is not on the DOM yet. | ||
| // However, we need to calculate number of possible items when the more button present as well. So using the width of the more button as a constant. | ||
|
|
@@ -90,7 +87,6 @@ const overflowEffect = ( | |
| const numberOfItemsInMenu = childArray.length - numberOfItemsPossibleWithMoreMenu | ||
| const numberOfListItems = | ||
| numberOfItemsInMenu === 1 ? numberOfItemsPossibleWithMoreMenu - 1 : numberOfItemsPossibleWithMoreMenu | ||
| for (const [index, child] of childArray.entries()) { | ||
| if (index < numberOfListItems) { | ||
| items.push(child) | ||
|
|
@@ -120,15 +116,15 @@ const getValidChildren = (children: React.ReactNode) => { | |
|
|
||
| const calculatePossibleItems = (childWidthArray: ChildWidthArray, navWidth: number, moreMenuWidth = 0) => { | ||
| const widthToFit = navWidth - moreMenuWidth | ||
| let breakpoint = childWidthArray.length // assume all items will fit | ||
| let sumsOfChildWidth = 0 | ||
| for (const [index, childWidth] of childWidthArray.entries()) { | ||
| sumsOfChildWidth = sumsOfChildWidth + childWidth.width + GAP | ||
| if (sumsOfChildWidth > widthToFit) { | ||
| breakpoint = index | ||
| break | ||
| } else { | ||
| continue | ||
|
Comment on lines
+122
to
+127
There was a problem hiding this comment. I think calculating the sum first and then to see if that will fit into the available width is a better approach. It fixes this specific overflow issue |
||
| } | ||
| } | ||
| return breakpoint | ||
|
|
@@ -138,11 +134,8 @@ export const UnderlineNav = forwardRef( | |
| ( | ||
| { | ||
| as = 'nav', | ||
| 'aria-label': ariaLabel, | ||
| sx: sxProp = defaultSxProp, | ||
| loadingCounters = false, | ||
| children, | ||
| }: UnderlineNavProps, | ||
|
|
@@ -165,7 +158,7 @@ export const UnderlineNav = forwardRef( | |
| const swapMenuItemWithListItem = ( | ||
| prospectiveListItem: React.ReactElement, | ||
| indexOfProspectiveListItem: number, | ||
| event: React.MouseEvent<HTMLAnchorElement> | React.KeyboardEvent<HTMLAnchorElement>, | ||
|
There was a problem hiding this comment. |
||
| callback: (props: ResponsiveProps, displayIcons: boolean) => void, | ||
| ) => { | ||
| // get the selected menu item's width | ||
|
|
@@ -186,7 +179,6 @@ export const UnderlineNav = forwardRef( | |
| const updatedMenuItems = [...actions] | ||
| // Add itemsToAddToMenu array's items to the menu at the index of the prospectiveListItem and remove 1 count of items (prospectiveListItem) | ||
| updatedMenuItems.splice(indexOfProspectiveListItem, 1, ...itemsToAddToMenu) | ||
| callback({items: updatedItemList, actions: updatedMenuItems}, false) | ||
| } | ||
| // How many items do we need to pull in to the menu to make room for the selected menu item. | ||
|
|
@@ -203,25 +195,8 @@ export const UnderlineNav = forwardRef( | |
| return breakpoint | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. We don't really need |
||
|
Comment on lines
-208
to
-215
There was a problem hiding this comment. This was a bad solution 🙈 I implemented this a way back and now looking at it now, triggering onSelect function on the selected menu item is as straightforward as this |
||
| const [iconsVisible, setIconsVisible] = useState<boolean>(true) | ||
|
|
||
|
There was a problem hiding this comment. After removing the |
||
| const [responsiveProps, setResponsiveProps] = useState<ResponsiveProps>({ | ||
| items: getValidChildren(children), | ||
| actions: [], | ||
|
|
@@ -232,30 +207,30 @@ export const UnderlineNav = forwardRef( | |
| * Particually when an item is selected. It adds 'aria-current="page"' attribute to the child and we need to make sure | ||
| * responsiveProps.items and ResponsiveProps.actions are updated with that attribute | ||
| */ | ||
| const listLinks = responsiveProps.items.map(item => { | ||
| return getValidChildren(children).find(child => child.key === item.key) || item | ||
| }) | ||
| // TODO: Rename this to menuItems | ||
| const actions = responsiveProps.actions.map(action => { | ||
| return getValidChildren(children).find(child => child.key === action.key) || action | ||
| }) | ||
|
|
||
| const updateListAndMenu = useCallback((props: ResponsiveProps, displayIcons: boolean) => { | ||
| setResponsiveProps(props) | ||
| setIconsVisible(displayIcons) | ||
| }, []) | ||
|
|
||
| // Address illegal state where there are multiple items that have `aria-current=''page'` attribute | ||
| if (__DEV__) { | ||
| // Practically, this is not a conditional hook, it is just making sure this hook runs only on DEV not PROD. | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| useEffect(() => { | ||
| const activeElements = getValidChildren(children).filter(child => { | ||
| return child.props['aria-current'] !== undefined | ||
| }) | ||
| invariant(activeElements.length <= 1, 'Only one current element is allowed') | ||
| }) | ||
| } | ||
| // This is the case where the viewport is too narrow to show any list item with the more menu. In this case, we only show the dropdown | ||
| const onlyMenuVisible = responsiveProps.items.length === 0 | ||
| const [childWidthArray, setChildWidthArray] = useState<ChildWidthArray>([]) | ||
|
|
@@ -321,26 +296,30 @@ export const UnderlineNav = forwardRef( | |
| theme, | ||
| setChildrenWidth, | ||
| setNoIconChildrenWidth, | ||
| loadingCounters, | ||
| iconsVisible, | ||
| }} | ||
| > | ||
| {ariaLabel && <VisuallyHidden as="h2">{`${ariaLabel} navigation`}</VisuallyHidden>} | ||
| <Box | ||
| as={as} | ||
| sx={merge<BetterSystemStyleObject>(getNavStyles(theme), sxProp)} | ||
| aria-label={ariaLabel} | ||
| ref={navRef} | ||
| > | ||
| <NavigationList sx={ulStyles} ref={listRef} role="list"> | ||
| {listLinks.map(listLink => { | ||
| return ( | ||
| <Box | ||
|
There was a problem hiding this comment. I moved the Box wrapper here to make UnderlineNav return |
||
| key={listLink.props.children} | ||
| as="li" | ||
| sx={{display: 'flex', flexDirection: 'column', alignItems: 'center'}} | ||
| > | ||
| {listLink} | ||
| </Box> | ||
| ) | ||
| })} | ||
|
|
||
| {actions.length > 0 && ( | ||
| <MoreMenuListItem ref={moreMenuRef}> | ||
| {!onlyMenuVisible && <Box sx={getDividerStyle(theme)}></Box>} | ||
|
|
@@ -372,36 +351,56 @@ export const UnderlineNav = forwardRef( | |
| style={{display: isWidgetOpen ? 'block' : 'none'}} | ||
| > | ||
| {actions.map((action, index) => { | ||
| const { | ||
| children: actionElementChildren, | ||
| counter, | ||
| 'aria-current': ariaCurrent, | ||
| onSelect, | ||
| ...actionElementProps | ||
| } = action.props | ||
|
|
||
| // This logic is used to pop the selected item out of the menu and into the list when the navigation is control externally | ||
| if (Boolean(ariaCurrent) && ariaCurrent !== 'false') { | ||
|
There was a problem hiding this comment. please refer to the gif in the description to see this in action. |
||
| const event = new MouseEvent('click') | ||
| !onlyMenuVisible && | ||
| swapMenuItemWithListItem( | ||
| action, | ||
| index, | ||
| // @ts-ignore - not a big deal because it is internally creating an event but ask help | ||
| event as React.MouseEvent<HTMLAnchorElement>, | ||
| updateListAndMenu, | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <ActionList.LinkItem | ||
|
There was a problem hiding this comment. Refactor the component to use |
||
| key={actionElementChildren} | ||
| sx={menuItemStyles} | ||
| onClick={( | ||
| event: React.MouseEvent<HTMLAnchorElement> | React.KeyboardEvent<HTMLAnchorElement>, | ||
| ) => { | ||
| // When there are no items in the list, do not run the swap function as we want to keep everything in the menu. | ||
| !onlyMenuVisible && swapMenuItemWithListItem(action, index, event, updateListAndMenu) | ||
| closeOverlay() | ||
| focusOnMoreMenuBtn() | ||
| // fire onSelect event that comes from the UnderlineNav.Item (if it is defined) | ||
| typeof onSelect === 'function' && onSelect(event) | ||
| }} | ||
| {...actionElementProps} | ||
| > | ||
| <Box as="span" sx={{display: 'flex', alignItems: 'center', justifyContent: 'space-between'}}> | ||
| {actionElementChildren} | ||
| {loadingCounters ? ( | ||
| <LoadingCounter /> | ||
| ) : ( | ||
| counter !== undefined && ( | ||
| <Box as="span" data-component="counter"> | ||
| <CounterLabel>{counter}</CounterLabel> | ||
| </Box> | ||
| ) | ||
| )} | ||
| </Box> | ||
| </ActionList.LinkItem> | ||
| ) | ||
| })} | ||
| </ActionList> | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,16 @@ | ||
| import React, {createContext} from 'react' | ||
| import {Theme} from '../ThemeProvider' | ||
|
|
||
| export const UnderlineNavContext = createContext<{ | ||
| theme: Theme | undefined | ||
| setChildrenWidth: React.Dispatch<{text: string; width: number}> | ||
| setNoIconChildrenWidth: React.Dispatch<{text: string; width: number}> | ||
| loadingCounters: boolean | ||
| iconsVisible: boolean | ||
| }>({ | ||
| theme: {}, | ||
| setChildrenWidth: () => null, | ||
| setNoIconChildrenWidth: () => null, | ||
| loadingCounters: false, | ||
| iconsVisible: true, | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Note for reviewers:
variantandalignwere considered in the initial version of the component but it is being decided not to include them at the end. Removing them to reduce the clutter and complexity on the component and styles.