From 73c3d4a1c4eee7c67455b8d515b0176930eb35f9 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Tue, 10 May 2022 18:23:31 -0700 Subject: [PATCH 01/19] wip subnav --- src/NavList/NavList.stories.tsx | 35 ++++++++++ src/NavList/NavList.tsx | 115 +++++++++++++++++++++++++++++++- 2 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 src/NavList/NavList.stories.tsx diff --git a/src/NavList/NavList.stories.tsx b/src/NavList/NavList.stories.tsx new file mode 100644 index 00000000000..bdd8abc256d --- /dev/null +++ b/src/NavList/NavList.stories.tsx @@ -0,0 +1,35 @@ +import {Meta, Story} from '@storybook/react' +import React from 'react' +import {PageLayout} from '../PageLayout' +import {NavList} from './NavList' + +const meta: Meta = { + title: 'Composite components/NavList', + component: NavList, + parameters: { + layout: 'fullscreen' + } +} + +export const SubItems: Story = () => ( + + + + Item 1 + + Item 2 + + + Sub item 1 + + Sub item 2 + + + Item 3 + + + + +) + +export default meta diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index f3329181e8d..98717058214 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -1,5 +1,9 @@ -import React from 'react' +import {ChevronDownIcon} from '@primer/octicons-react' +import {useSSRSafeId} from '@react-aria/ssr' +import React, {isValidElement} from 'react' import {ActionList} from '../ActionList' +import Box from '../Box' +import StyledOcticon from '../StyledOcticon' // ---------------------------------------------------------------------------- // NavList @@ -29,8 +33,32 @@ export type NavListItemProps = { // sx } +const ItemContext = React.createContext<{depth: number}>({depth: 0}) + const Item = React.forwardRef( ({href, 'aria-current': ariaCurrent, children}, ref) => { + const {depth} = React.useContext(ItemContext) + + // TODO: Test two-level nav list + if (depth > 1) { + // eslint-disable-next-line no-console + console.error('NavList only supports one level of nesting') + return null + } + + // Get SubNav from children + const subNav = React.Children.toArray(children).find(child => isValidElement(child) && child.type === SubNav) + + // Get children without SubNav + const childrenWithoutSubNav = React.Children.toArray(children).filter(child => + isValidElement(child) ? child.type !== SubNav : true + ) + + // Render ItemWithSubNav if SubNav is present + if (subNav) { + return {childrenWithoutSubNav} + } + return ( ( aria-current={ariaCurrent} sx={{ position: 'relative', + paddingLeft: depth === 1 ? 5 : null, // Indent sub-items + fontSize: depth === 1 ? 0 : null, // Reduce font size of sub-items '&[aria-current]': { - fontWeight: 'bold', + fontWeight: depth === 0 ? 'bold' : null, // Sub-items don't get bolded bg: 'actionListItem.default.selectedBg', '&::after': { position: 'absolute', @@ -60,6 +90,86 @@ const Item = React.forwardRef( } ) +Item.displayName = 'NavList.Item' + +// ---------------------------------------------------------------------------- +// ItemWithSubNav (internal) + +type ItemWithSubNavProps = { + children: React.ReactNode + subNav: React.ReactNode + // sx +} + +const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: string}>({buttonId: '', subNavId: ''}) + +// TODO: Forward ref +function ItemWithSubNav({children, subNav}: ItemWithSubNavProps) { + const buttonId = useSSRSafeId() + const subNavId = useSSRSafeId() + const subNavRef = React.useRef(null) + const [isOpen, setIsOpen] = React.useState(false) + // TODO: Check if subNav contains aria-current + // TODO: Animation + return ( + + + {/* TODO: parent of aria-current should be bold, and have active styles if closed */} + setIsOpen(open => !open)} + > + {children} + {/* What happens if the user provides a TrailingVisual? */} + + + + + + {isOpen ?
{subNav}
: null} + + + ) +} + +// ---------------------------------------------------------------------------- +// NavList.SubNav + +type NavListSubNavProps = { + children: React.ReactNode + // sx +} + +// SubNav must be a direct child of an Item +// TODO: Forward ref +const SubNav = ({children}: NavListSubNavProps) => { + const {buttonId, subNavId} = React.useContext(ItemWithSubNavContext) + const {depth} = React.useContext(ItemContext) + + if (!buttonId || !subNavId) { + // eslint-disable-next-line no-console + console.error('NavList.SubNav must be a child of a NavList.Item') + } + + return ( + + + {children} + + + ) +} + +SubNav.displayName = 'NavList.SubNav' + // ---------------------------------------------------------------------------- // NavList.LeadingVisual @@ -108,6 +218,7 @@ Group.displayName = 'NavList.Group' export const NavList = Object.assign(Root, { Item, + SubNav, LeadingVisual, TrailingVisual, Divider, From 9c52b234e69b48013b588c271da0c936f3f35b54 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 11 May 2022 15:53:51 -0700 Subject: [PATCH 02/19] Add active prop to ActionList.Item --- src/ActionList/Item.tsx | 32 ++++++++++++++++++++++++++------ src/ActionList/LinkItem.tsx | 5 +++-- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index c57eba32b85..97e5825c545 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -1,15 +1,15 @@ -import React from 'react' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '@radix-ui/react-polymorphic' import {useSSRSafeId} from '@react-aria/ssr' +import React from 'react' import styled from 'styled-components' -import {useTheme} from '../ThemeProvider' import Box, {BoxProps} from '../Box' -import sx, {SxProp, merge} from '../sx' +import sx, {merge, SxProp} from '../sx' +import {useTheme} from '../ThemeProvider' import createSlots from '../utils/create-slots' import {AriaRole} from '../utils/types' -import {ListContext, ActionListProps} from './List' -import {GroupContext, ActionListGroupProps} from './Group' import {ActionListContainerContext} from './ActionListContainerContext' +import {ActionListGroupProps, GroupContext} from './Group' +import {ActionListProps, ListContext} from './List' import {Selection} from './Selection' export const getVariantStyles = ( @@ -55,6 +55,8 @@ export type ActionListItemProps = { * Is the `Item` is currently selected? */ selected?: boolean + // TODO: Document `active` prop + active?: boolean /** * Style variations associated with various `Item` types. * @@ -96,6 +98,7 @@ export const Item = React.forwardRef( variant = 'default', disabled = false, selected = undefined, + active = false, onSelect, sx: sxProp = {}, id, @@ -123,7 +126,23 @@ export const Item = React.forwardRef( const {theme} = useTheme() + const activeStyles = { + fontWeight: 'bold', + bg: 'actionListItem.default.selectedBg', + '&::after': { + position: 'absolute', + top: 'calc(50% - 12px)', + left: '-8px', + width: '4px', + height: '24px', + content: '""', + bg: 'accent.fg', + borderRadius: 2 + } + } + const styles = { + position: 'relative', display: 'flex', paddingX: 2, fontSize: 1, @@ -190,7 +209,8 @@ export const Item = React.forwardRef( }, '&:hover:not([aria-disabled]) + &, &:focus:not([aria-disabled]) + &, &[data-focus-visible-added] + li': { '--divider-color': 'transparent' - } + }, + ...(active ? activeStyles : {}) } const clickHandler = React.useCallback( diff --git a/src/ActionList/LinkItem.tsx b/src/ActionList/LinkItem.tsx index 7ee84b699d1..b7ab955f7d2 100644 --- a/src/ActionList/LinkItem.tsx +++ b/src/ActionList/LinkItem.tsx @@ -18,9 +18,9 @@ type LinkProps = { } // LinkItem does not support selected, variants, etc. -export type ActionListLinkItemProps = Pick & LinkProps +export type ActionListLinkItemProps = Pick & LinkProps -export const LinkItem = React.forwardRef(({sx = {}, as: Component, ...props}, forwardedRef) => { +export const LinkItem = React.forwardRef(({sx = {}, active, as: Component, ...props}, forwardedRef) => { const styles = { // occupy full size of Item paddingX: 2, @@ -36,6 +36,7 @@ export const LinkItem = React.forwardRef(({sx = {}, as: Component, ...props}, fo return ( ( From 3cc54c05a9771a39689ff683ab997389178052b1 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 11 May 2022 15:54:23 -0700 Subject: [PATCH 03/19] Handle nested current item --- src/NavList/NavList.tsx | 76 ++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index 98717058214..c5df1b190b3 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -10,9 +10,9 @@ import StyledOcticon from '../StyledOcticon' export type NavListProps = { children: React.ReactNode - // sx } & React.ComponentProps<'nav'> +// TODO: sx prop const Root = React.forwardRef(({children, ...props}, ref) => { return ( From 9dd1e798707178302fd4981b025bb681543b9ffe Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 12 May 2022 12:51:42 -0700 Subject: [PATCH 13/19] Add nested subnav test --- src/NavList/NavList.test.tsx | 41 ++++++++++++++++++++++++++++++++---- src/NavList/NavList.tsx | 34 +++++++++++++++--------------- 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/src/NavList/NavList.test.tsx b/src/NavList/NavList.test.tsx index d3fa8594909..8acb971d40c 100644 --- a/src/NavList/NavList.test.tsx +++ b/src/NavList/NavList.test.tsx @@ -110,9 +110,15 @@ describe('NavList.Item with NavList.SubNav', () => { }) it('shows SubNav by default if SubNav contains the current item', () => { - const {queryByRole} = render() - const subNav = queryByRole('list', {name: 'Item 2'}) - expect(subNav).toBeVisible() + const {queryByRole, getByRole} = render() + + // Starts open + expect(queryByRole('list', {name: 'Item 2'})).toBeVisible() + + // Click to close + const itemWithSubNav = getByRole('button', {name: 'Item 2'}) + fireEvent.click(itemWithSubNav) + expect(queryByRole('list', {name: 'Item 2'})).toBeNull() }) it('hides SubNav by default if SubNav does not contain the current item', () => { @@ -193,5 +199,32 @@ describe('NavList.Item with NavList.SubNav', () => { expect(container).toMatchSnapshot() }) - it.todo('prints an error if Item contains multiple nested SubNavs') + it('prevents multiple levels of nested SubNavs', () => { + const consoleSpy = jest + .spyOn(console, 'error') + // Suppress error message in test output + .mockImplementation(() => null) + + const {getByRole} = render( + + + Item + + + Sub item + {/* NOTE: Don't nest SubNavs. For testing purposes only */} + + Sub sub item + + + + + + ) + + const item = getByRole('button', {name: 'Item'}) + fireEvent.click(item) + + expect(consoleSpy).toHaveBeenCalled() + }) }) diff --git a/src/NavList/NavList.tsx b/src/NavList/NavList.tsx index d8a49ed4fdc..5cea60fb008 100644 --- a/src/NavList/NavList.tsx +++ b/src/NavList/NavList.tsx @@ -32,20 +32,11 @@ export type NavListItemProps = { 'aria-current'?: 'page' | 'step' | 'location' | 'date' | 'time' | 'true' | 'false' | boolean } -const ItemContext = React.createContext<{depth: number}>({depth: 0}) - // TODO: sx prop // TODO: as prop const Item = React.forwardRef( ({href, 'aria-current': ariaCurrent, children}, ref) => { - const {depth} = React.useContext(ItemContext) - - // TODO: Test this error case - if (depth > 1) { - // eslint-disable-next-line no-console - console.error('NavList only supports one level of nesting') - return null - } + const {depth} = React.useContext(SubNavContext) // Get SubNav from children const subNav = React.Children.toArray(children).find(child => isValidElement(child) && child.type === SubNav) @@ -56,7 +47,7 @@ const Item = React.forwardRef( ) // Render ItemWithSubNav if SubNav is present - if (subNav && isValidElement(subNav)) { + if (subNav && isValidElement(subNav) && depth < 1) { // Search SubNav children for current Item const currentItem = React.Children.toArray(subNav.props.children).find( child => isValidElement(child) && child.props['aria-current'] @@ -76,9 +67,9 @@ const Item = React.forwardRef( aria-current={ariaCurrent} active={Boolean(ariaCurrent) && ariaCurrent !== 'false'} sx={{ - paddingLeft: depth === 1 ? 5 : null, // Indent sub-items - fontSize: depth === 1 ? 0 : null, // Reduce font size of sub-items - fontWeight: depth === 1 ? 'normal' : null // Sub-items don't get bolded + paddingLeft: depth > 0 ? 5 : null, // Indent sub-items + fontSize: depth > 0 ? 0 : null, // Reduce font size of sub-items + fontWeight: depth > 0 ? 'normal' : null // Sub-items don't get bolded }} > {children} @@ -111,6 +102,7 @@ function ItemWithSubNav({children, subNav, subNavContainsCurrentItem}: ItemWithS const subNavId = useSSRSafeId() // SubNav starts open if current item is in it const [isOpen, setIsOpen] = React.useState(subNavContainsCurrentItem) + return ( @@ -151,24 +143,32 @@ type NavListSubNavProps = { children: React.ReactNode } +const SubNavContext = React.createContext<{depth: number}>({depth: 0}) + // TODO: sx prop // TODO: ref prop // NOTE: SubNav must be a direct child of an Item const SubNav = ({children}: NavListSubNavProps) => { const {buttonId, subNavId} = React.useContext(ItemWithSubNavContext) - const {depth} = React.useContext(ItemContext) + const {depth} = React.useContext(SubNavContext) if (!buttonId || !subNavId) { // eslint-disable-next-line no-console console.error('NavList.SubNav must be a child of a NavList.Item') } + if (depth > 0) { + // eslint-disable-next-line no-console + console.error('NavList.SubNav only supports one level of nesting') + return null + } + return ( - + {children} - + ) } From 8fc40e6d30305d9cd0ebb51de3d84bd9f0b0fe9b Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 12 May 2022 12:58:58 -0700 Subject: [PATCH 14/19] Update docs --- docs/content/NavList.mdx | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/docs/content/NavList.mdx b/docs/content/NavList.mdx index 4521b2a9d31..1fb38fd7765 100644 --- a/docs/content/NavList.mdx +++ b/docs/content/NavList.mdx @@ -135,16 +135,16 @@ import {NavList} from '@primer/react/drafts' ### With sub-items -Not implemented yet - -```jsx +```jsx live drafts Branches Tags Actions - General + + General + Runners @@ -157,30 +157,6 @@ If a `NavList.Item` contains a `NavList.SubNav`, the `NavList.Item` will render -
- Rendered HTML - -```html - -``` - -
- ### With a divider ```jsx live drafts From 293fe76a38969a8a9351a238226c13fe8ba71ed0 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 12 May 2022 13:05:43 -0700 Subject: [PATCH 15/19] Use spacing primitive --- src/ActionList/Item.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index c844150e5d9..09d8bc62868 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -132,7 +132,7 @@ export const Item = React.forwardRef( '&::after': { position: 'absolute', top: 'calc(50% - 12px)', - left: '-8px', + left: -2, width: '4px', height: '24px', content: '""', From 64c76970df6d6a7bd43ae128f010cc6b9172d023 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 12 May 2022 13:05:57 -0700 Subject: [PATCH 16/19] Reset marginY for safari --- src/ActionList/Item.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index 09d8bc62868..c6cac5f41d6 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -163,6 +163,7 @@ export const Item = React.forwardRef( width: '100%', fontFamily: 'unset', textAlign: 'unset', + marginY: 'unset', '@media (hover: hover) and (pointer: fine)': { ':hover:not([aria-disabled])': { From 30786fb5e84c08269d3513f101e0510890a68634 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 12 May 2022 13:16:45 -0700 Subject: [PATCH 17/19] Document active prop --- docs/content/ActionList.mdx | 12 ++++++++++++ src/ActionList/Item.tsx | 4 +++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/docs/content/ActionList.mdx b/docs/content/ActionList.mdx index 9ee2161e640..227be633698 100644 --- a/docs/content/ActionList.mdx +++ b/docs/content/ActionList.mdx @@ -297,6 +297,12 @@ render() defaultValue="false" description="Indicate whether the item is selected. Only applies to items that can be selected." /> + ) required type="React.ReactNode | ActionList.LeadingVisual | ActionList.Description | ActionList.TrailingVisual" /> + Date: Thu, 12 May 2022 13:20:58 -0700 Subject: [PATCH 18/19] Create strong-nails-sip.md --- .changeset/strong-nails-sip.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/strong-nails-sip.md diff --git a/.changeset/strong-nails-sip.md b/.changeset/strong-nails-sip.md new file mode 100644 index 00000000000..c6a801bd390 --- /dev/null +++ b/.changeset/strong-nails-sip.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Add support for sub-items in draft implementation of NavList From 8e5dd1b6813bc31dbd732951e89ca64776d3b258 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 12 May 2022 13:46:21 -0700 Subject: [PATCH 19/19] Update snapshots --- src/NavList/__snapshots__/NavList.test.tsx.snap | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap index 4fbd25a466b..1b9ef3098c1 100644 --- a/src/NavList/__snapshots__/NavList.test.tsx.snap +++ b/src/NavList/__snapshots__/NavList.test.tsx.snap @@ -58,6 +58,8 @@ exports[`NavList renders a simple list 1`] = ` width: 100%; font-family: unset; text-align: unset; + margin-top: unset; + margin-bottom: unset; font-weight: 600; background-color: rgba(208,215,222,0.24); } @@ -140,6 +142,8 @@ exports[`NavList renders a simple list 1`] = ` width: 100%; font-family: unset; text-align: unset; + margin-top: unset; + margin-bottom: unset; } .c6[aria-disabled] { @@ -455,6 +459,8 @@ exports[`NavList renders with groups 1`] = ` width: 100%; font-family: unset; text-align: unset; + margin-top: unset; + margin-bottom: unset; font-weight: 600; background-color: rgba(208,215,222,0.24); } @@ -537,6 +543,8 @@ exports[`NavList renders with groups 1`] = ` width: 100%; font-family: unset; text-align: unset; + margin-top: unset; + margin-bottom: unset; } .c10[aria-disabled] { @@ -883,6 +891,8 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav width: 100%; font-family: unset; text-align: unset; + margin-top: unset; + margin-bottom: unset; font-weight: 600; background-color: rgba(208,215,222,0.24); } @@ -1001,6 +1011,8 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav width: 100%; font-family: unset; text-align: unset; + margin-top: unset; + margin-bottom: unset; font-weight: 600; } @@ -1381,6 +1393,8 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t width: 100%; font-family: unset; text-align: unset; + margin-top: unset; + margin-bottom: unset; font-weight: 600; background-color: rgba(208,215,222,0.24); }