From 7980fe1c6befd50871d8e861058070cedc839a6c Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Tue, 1 Aug 2023 12:45:04 +1000 Subject: [PATCH 1/9] Add ActionList.Heading component --- .changeset/slimy-starfishes-agree.md | 7 ++ .../ActionList.features.stories.tsx | 104 ++++++++++++++++++ src/ActionList/Heading.tsx | 42 +++++++ src/ActionList/List.tsx | 51 ++++++--- src/ActionList/index.ts | 5 + 5 files changed, 191 insertions(+), 18 deletions(-) create mode 100644 .changeset/slimy-starfishes-agree.md create mode 100644 src/ActionList/Heading.tsx diff --git a/.changeset/slimy-starfishes-agree.md b/.changeset/slimy-starfishes-agree.md new file mode 100644 index 00000000000..ddd43401776 --- /dev/null +++ b/.changeset/slimy-starfishes-agree.md @@ -0,0 +1,7 @@ +--- +'@primer/react': minor +--- + +ActionList: Add ActionList.Heading component + + diff --git a/src/ActionList/ActionList.features.stories.tsx b/src/ActionList/ActionList.features.stories.tsx index a27e2181d3c..d3c11a41e09 100644 --- a/src/ActionList/ActionList.features.stories.tsx +++ b/src/ActionList/ActionList.features.stories.tsx @@ -1,5 +1,6 @@ import React from 'react' import {Meta} from '@storybook/react' +import {ActionMenu} from '../ActionMenu' import {ActionList} from '.' import {Item} from './Item' import {LinkItem} from './LinkItem' @@ -41,6 +42,109 @@ export const SimpleList = () => ( ) +export const WithHeading = () => ( + + Details + + + + + Readme + + + + + + MIT License + + + + + + 1.5k stars + + + + + + 21 watching + + + + + + 225 forks + + +) + +export const WithHeadingInMenu = () => ( + + Open menu + + + Details + alert('Copy link clicked')}> + Copy link + ⌘C + + alert('Quote reply clicked')}> + Quote reply + ⌘Q + + alert('Edit comment clicked')}> + Edit comment + ⌘E + + + alert('Delete file clicked')}> + Delete file + ⌘D + + + + +) + +export const WithCustomHeading = () => ( + <> + + Details + + + + + + + Readme + + + + + + MIT License + + + + + + 1.5k stars + + + + + + 21 watching + + + + + + 225 forks + + + +) export const WithIcons = () => ( diff --git a/src/ActionList/Heading.tsx b/src/ActionList/Heading.tsx new file mode 100644 index 00000000000..567dbb649cc --- /dev/null +++ b/src/ActionList/Heading.tsx @@ -0,0 +1,42 @@ +import React, {forwardRef} from 'react' +import {BetterSystemStyleObject, SxProp, merge} from '../sx' +import {defaultSxProp} from '../utils/defaultSxProp' +import {useRefObjectAsForwardedRef} from '../hooks' +import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {default as HeadingComponent} from '../Heading' +import {ListContext} from './List' + +export type ActionListHeadingProps = { + as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' +} & SxProp + +export const Heading = forwardRef(({as: Component = 'h3', children, sx = defaultSxProp, ...props}, forwardedRef) => { + const innerRef = React.useRef(null) + useRefObjectAsForwardedRef(forwardedRef, innerRef) + + const {headingId: headingId} = React.useContext(ListContext) + + const styles = { + paddingY: '6px', + paddingX: 2, + fontSize: 0, + fontWeight: 'bold', + color: 'fg.muted', + listStyle: 'none', + } + + return ( + (styles, sx)} + {...props} + > + {children} + + ) +}) as PolymorphicForwardRefComponent<'h3', ActionListHeadingProps> + +Heading.displayName = 'ActionList.Heading' diff --git a/src/ActionList/List.tsx b/src/ActionList/List.tsx index 29bc2f54953..7815407e206 100644 --- a/src/ActionList/List.tsx +++ b/src/ActionList/List.tsx @@ -1,10 +1,12 @@ -import React from 'react' +import React, {useId} from 'react' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import styled from 'styled-components' import sx, {SxProp, merge} from '../sx' import {AriaRole} from '../utils/types' import {ActionListContainerContext} from './ActionListContainerContext' import {defaultSxProp} from '../utils/defaultSxProp' +import {useSlots} from '../hooks/useSlots' +import {Heading} from './Heading' export type ActionListProps = React.PropsWithChildren<{ /** @@ -26,7 +28,10 @@ export type ActionListProps = React.PropsWithChildren<{ }> & SxProp -type ContextProps = Pick +type ContextProps = Pick & { + headingId?: string +} + export const ListContext = React.createContext({}) const ListBox = styled.ul(sx) @@ -42,6 +47,12 @@ export const List = React.forwardRef( paddingY: variant === 'inset' ? 2 : 0, } + const [slots, childrenWithoutSlots] = useSlots(props.children, { + heading: Heading, + }) + + const headingId = useId() + /** if list is inside a Menu, it will get a role from the Menu */ const { listRole, @@ -49,25 +60,29 @@ export const List = React.forwardRef( selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation } = React.useContext(ActionListContainerContext) + const ariaLabelledBy = slots.heading ? slots.heading.props.id ?? headingId : listLabelledBy + return ( - - - {props.children} - - + {childrenWithoutSlots} + + ) }, ) as PolymorphicForwardRefComponent<'ul', ActionListProps> diff --git a/src/ActionList/index.ts b/src/ActionList/index.ts index c87e5bc7f65..202a96a1a4b 100644 --- a/src/ActionList/index.ts +++ b/src/ActionList/index.ts @@ -5,6 +5,7 @@ import {LinkItem} from './LinkItem' import {Divider} from './Divider' import {Description} from './Description' import {LeadingVisual, TrailingVisual} from './Visuals' +import {Heading} from './Heading' export type {ActionListProps} from './List' export type {ActionListGroupProps} from './Group' @@ -13,6 +14,7 @@ export type {ActionListLinkItemProps} from './LinkItem' export type {ActionListDividerProps} from './Divider' export type {ActionListDescriptionProps} from './Description' export type {ActionListLeadingVisualProps, ActionListTrailingVisualProps} from './Visuals' +export type {ActionListHeadingProps} from './Heading' /** * Collection of list-related components. @@ -38,4 +40,7 @@ export const ActionList = Object.assign(List, { /** Icon (or similar) positioned after `Item` text. */ TrailingVisual, + + /** Heading for an `ActionList`. */ + Heading, }) From 5f1a32230b9fdfa0c9393ff1eb8a67405e317d6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arma=C4=9Fan?= Date: Tue, 1 Aug 2023 13:11:26 +1000 Subject: [PATCH 2/9] Update .changeset/slimy-starfishes-agree.md --- .changeset/slimy-starfishes-agree.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/slimy-starfishes-agree.md b/.changeset/slimy-starfishes-agree.md index ddd43401776..be22781c088 100644 --- a/.changeset/slimy-starfishes-agree.md +++ b/.changeset/slimy-starfishes-agree.md @@ -4,4 +4,4 @@ ActionList: Add ActionList.Heading component - + From 576161f4a884817fe535e1d9a5439cbe5f9261b7 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 9 Aug 2023 11:22:53 +1000 Subject: [PATCH 3/9] heading styles --- .../ActionList.features.stories.tsx | 36 ++++++++++ src/ActionList/Heading.tsx | 68 ++++++++++++------- 2 files changed, 79 insertions(+), 25 deletions(-) diff --git a/src/ActionList/ActionList.features.stories.tsx b/src/ActionList/ActionList.features.stories.tsx index d3c11a41e09..9a8d4f4de0e 100644 --- a/src/ActionList/ActionList.features.stories.tsx +++ b/src/ActionList/ActionList.features.stories.tsx @@ -78,6 +78,42 @@ export const WithHeading = () => ( ) +export const WithHeadingInFullVariant = () => ( + + Details + + + + + Readme + + + + + + MIT License + + + + + + 1.5k stars + + + + + + 21 watching + + + + + + 225 forks + + +) + export const WithHeadingInMenu = () => ( Open menu diff --git a/src/ActionList/Heading.tsx b/src/ActionList/Heading.tsx index 567dbb649cc..59d1896136c 100644 --- a/src/ActionList/Heading.tsx +++ b/src/ActionList/Heading.tsx @@ -8,35 +8,53 @@ import {ListContext} from './List' export type ActionListHeadingProps = { as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' + /** + * Style variations. Usage is discretionary. + * + * - `"filled"` - Superimposed on a background, offset from nearby content + * - `"subtle"` - Relatively less offset from nearby content + */ + variant?: 'subtle' | 'filled' } & SxProp -export const Heading = forwardRef(({as: Component = 'h3', children, sx = defaultSxProp, ...props}, forwardedRef) => { - const innerRef = React.useRef(null) - useRefObjectAsForwardedRef(forwardedRef, innerRef) +export const Heading = forwardRef( + ({as: Component = 'h3', children, sx = defaultSxProp, variant = 'subtle', ...props}, forwardedRef) => { + const innerRef = React.useRef(null) + useRefObjectAsForwardedRef(forwardedRef, innerRef) - const {headingId: headingId} = React.useContext(ListContext) + const {headingId: headingId, variant: listVariant} = React.useContext(ListContext) - const styles = { - paddingY: '6px', - paddingX: 2, - fontSize: 0, - fontWeight: 'bold', - color: 'fg.muted', - listStyle: 'none', - } + const styles = { + paddingY: 2, + paddingX: listVariant === 'full' ? 2 : 3, + fontSize: 0, + fontWeight: 'bold', + color: 'fg.muted', + listStyle: 'none', + lineHeight: 'calc(20 / 12)', + ...(variant === 'filled' && { + backgroundColor: 'canvas.subtle', + marginX: 0, + marginBottom: 2, + borderTop: '1px solid', + borderBottom: '1px solid', + borderColor: 'neutral.muted', + }), + } - return ( - (styles, sx)} - {...props} - > - {children} - - ) -}) as PolymorphicForwardRefComponent<'h3', ActionListHeadingProps> + return ( + (styles, sx)} + {...props} + > + {children} + + ) + }, +) as PolymorphicForwardRefComponent<'h3', ActionListHeadingProps> Heading.displayName = 'ActionList.Heading' From 5a316b5f8bd61915e8c23f22edd71e749072741d Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 1 Sep 2023 12:18:34 +1000 Subject: [PATCH 4/9] Update heading style and add visually hidden wrapper with story updates --- .../ActionList.features.stories.tsx | 113 ++++++++---------- src/ActionList/Heading.tsx | 50 +++----- 2 files changed, 69 insertions(+), 94 deletions(-) diff --git a/src/ActionList/ActionList.features.stories.tsx b/src/ActionList/ActionList.features.stories.tsx index 373ac6f605b..1a971a53cd6 100644 --- a/src/ActionList/ActionList.features.stories.tsx +++ b/src/ActionList/ActionList.features.stories.tsx @@ -24,6 +24,8 @@ import { AlertIcon, TableIcon, PeopleIcon, + FileDirectoryIcon, + PlusCircleIcon, } from '@primer/octicons-react' export default { @@ -42,39 +44,56 @@ export const SimpleList = () => ( ) -export const WithHeading = () => ( +export const WithVisualListHeading = () => ( - Details - - - - - Readme - - - - - - MIT License - - - - - - 1.5k stars - - - - - - 21 watching - - - - - - 225 forks - + Filter by + + {}}> + + + + app/assets/modules + + {}}> + + + + src/react/components + + {}}> + + + + memex/shared-ui/components + + {}}> + + + + views/assets/modules + + + + + {}}> + + + + Owner + + {}}> + + + + Symbol + + {}}> + + + + Exclude archived + + ) @@ -114,37 +133,9 @@ export const WithHeadingInFullVariant = () => ( ) -export const WithHeadingInMenu = () => ( - - Open menu - - - Details - alert('Copy link clicked')}> - Copy link - ⌘C - - alert('Quote reply clicked')}> - Quote reply - ⌘Q - - alert('Edit comment clicked')}> - Edit comment - ⌘E - - - alert('Delete file clicked')}> - Delete file - ⌘D - - - - -) - export const WithCustomHeading = () => ( <> - + Details diff --git a/src/ActionList/Heading.tsx b/src/ActionList/Heading.tsx index 59d1896136c..75161f58b49 100644 --- a/src/ActionList/Heading.tsx +++ b/src/ActionList/Heading.tsx @@ -5,54 +5,38 @@ import {useRefObjectAsForwardedRef} from '../hooks' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {default as HeadingComponent} from '../Heading' import {ListContext} from './List' +import VisuallyHidden from '../_VisuallyHidden' export type ActionListHeadingProps = { as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' - /** - * Style variations. Usage is discretionary. - * - * - `"filled"` - Superimposed on a background, offset from nearby content - * - `"subtle"` - Relatively less offset from nearby content - */ - variant?: 'subtle' | 'filled' + visuallyHidden?: boolean } & SxProp export const Heading = forwardRef( - ({as: Component = 'h3', children, sx = defaultSxProp, variant = 'subtle', ...props}, forwardedRef) => { + ({as: Component = 'h3', children, sx = defaultSxProp, visuallyHidden = false, ...props}, forwardedRef) => { const innerRef = React.useRef(null) useRefObjectAsForwardedRef(forwardedRef, innerRef) const {headingId: headingId, variant: listVariant} = React.useContext(ListContext) const styles = { - paddingY: 2, - paddingX: listVariant === 'full' ? 2 : 3, - fontSize: 0, - fontWeight: 'bold', - color: 'fg.muted', - listStyle: 'none', - lineHeight: 'calc(20 / 12)', - ...(variant === 'filled' && { - backgroundColor: 'canvas.subtle', - marginX: 0, - marginBottom: 2, - borderTop: '1px solid', - borderBottom: '1px solid', - borderColor: 'neutral.muted', - }), + marginBottom: 2, + marginX: listVariant === 'full' ? 2 : 3, } return ( - (styles, sx)} - {...props} - > - {children} - + + (styles, sx)} + {...props} + > + {children} + + ) }, ) as PolymorphicForwardRefComponent<'h3', ActionListHeadingProps> From 7f9711e212df6d0655c883d371230f72a124dc13 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 7 Sep 2023 16:19:51 +1000 Subject: [PATCH 5/9] warning for heading in menu & make as prop required --- .../ActionList.features.stories.tsx | 1 - .../ActionList.test.tsx | 53 ++++++++++++++++++- src/ActionList/Heading.tsx | 18 +++++-- .../__snapshots__/ActionList.test.tsx.snap | 0 4 files changed, 65 insertions(+), 7 deletions(-) rename src/{__tests__ => ActionList}/ActionList.test.tsx (74%) rename src/{__tests__ => ActionList}/__snapshots__/ActionList.test.tsx.snap (100%) diff --git a/src/ActionList/ActionList.features.stories.tsx b/src/ActionList/ActionList.features.stories.tsx index 1a971a53cd6..a4b9aeb48dd 100644 --- a/src/ActionList/ActionList.features.stories.tsx +++ b/src/ActionList/ActionList.features.stories.tsx @@ -1,6 +1,5 @@ import React from 'react' import {Meta} from '@storybook/react' -import {ActionMenu} from '../ActionMenu' import {ActionList} from '.' import {Item} from './Item' import {LinkItem} from './LinkItem' diff --git a/src/__tests__/ActionList.test.tsx b/src/ActionList/ActionList.test.tsx similarity index 74% rename from src/__tests__/ActionList.test.tsx rename to src/ActionList/ActionList.test.tsx index ca070d04305..e721ac432e3 100644 --- a/src/__tests__/ActionList.test.tsx +++ b/src/ActionList/ActionList.test.tsx @@ -2,9 +2,10 @@ import {render as HTMLRender, waitFor, fireEvent} from '@testing-library/react' import {axe} from 'jest-axe' import React from 'react' import theme from '../theme' -import {ActionList} from '../ActionList' +import {ActionList} from '.' import {behavesAsComponent, checkExports} from '../utils/testing' -import {BaseStyles, ThemeProvider, SSRProvider} from '..' +import {BaseStyles, ThemeProvider, SSRProvider, ActionMenu} from '..' +import userEvent from '@testing-library/user-event' function SimpleActionList(): JSX.Element { return ( @@ -166,4 +167,52 @@ describe('ActionList', () => { fireEvent.click(link) expect(onClick).toHaveBeenCalled() }) + + it('should render the ActionList.Heading component as a heading with the given heading level', async () => { + const container = HTMLRender( + + Heading + , + ) + const heading = container.getByRole('heading', {level: 1}) + expect(heading).toBeInTheDocument() + expect(heading).toHaveTextContent('Heading') + }) + it('should label the action list with the heading id', async () => { + const {container, getByRole} = HTMLRender( + + Heading + Item + , + ) + const list = container.querySelector('ul') + const heading = getByRole('heading', {level: 1}) + expect(list).toHaveAttribute('aria-labelledby', heading.id) + }) + it('should throw an error when ActionList.Heading is used within ActionMenu context', async () => { + const spy = jest.spyOn(console, 'error').mockImplementation(() => jest.fn()) + expect(() => + HTMLRender( + + + + + Trigger + + + Heading + Item + + + + + + , + ), + ).toThrow( + "ActionList.Heading shouldn't be used within an ActionMenu container. Menus are labelled by the menu button's name.", + ) + expect(spy).toHaveBeenCalled() + spy.mockRestore() + }) }) diff --git a/src/ActionList/Heading.tsx b/src/ActionList/Heading.tsx index 75161f58b49..076d690df28 100644 --- a/src/ActionList/Heading.tsx +++ b/src/ActionList/Heading.tsx @@ -6,18 +6,28 @@ import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/po import {default as HeadingComponent} from '../Heading' import {ListContext} from './List' import VisuallyHidden from '../_VisuallyHidden' +import {ActionListContainerContext} from './ActionListContainerContext' +import {invariant} from '../utils/invariant' +type HeadingLevels = 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' export type ActionListHeadingProps = { - as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' + as: HeadingLevels visuallyHidden?: boolean } & SxProp export const Heading = forwardRef( - ({as: Component = 'h3', children, sx = defaultSxProp, visuallyHidden = false, ...props}, forwardedRef) => { + ({as, children, sx = defaultSxProp, visuallyHidden = false, ...props}, forwardedRef) => { const innerRef = React.useRef(null) useRefObjectAsForwardedRef(forwardedRef, innerRef) const {headingId: headingId, variant: listVariant} = React.useContext(ListContext) + const {container} = React.useContext(ActionListContainerContext) + + // Semantic s don't have a place for headers within them, they should be aria-labelledby the menu button's name. + invariant( + container !== 'ActionMenu', + `ActionList.Heading shouldn't be used within an ActionMenu container. Menus are labelled by the menu button's name.`, + ) const styles = { marginBottom: 2, @@ -27,7 +37,7 @@ export const Heading = forwardRef( return ( ) }, -) as PolymorphicForwardRefComponent<'h3', ActionListHeadingProps> +) as PolymorphicForwardRefComponent Heading.displayName = 'ActionList.Heading' diff --git a/src/__tests__/__snapshots__/ActionList.test.tsx.snap b/src/ActionList/__snapshots__/ActionList.test.tsx.snap similarity index 100% rename from src/__tests__/__snapshots__/ActionList.test.tsx.snap rename to src/ActionList/__snapshots__/ActionList.test.tsx.snap From 77f536482e6aa9e68c8b825dbb7be253b656d617 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 7 Sep 2023 16:25:49 +1000 Subject: [PATCH 6/9] =?UTF-8?q?fix=20linting=20=F0=9F=99=84?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/ActionList/ActionList.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ActionList/ActionList.test.tsx b/src/ActionList/ActionList.test.tsx index e721ac432e3..5613411dda3 100644 --- a/src/ActionList/ActionList.test.tsx +++ b/src/ActionList/ActionList.test.tsx @@ -5,7 +5,6 @@ import theme from '../theme' import {ActionList} from '.' import {behavesAsComponent, checkExports} from '../utils/testing' import {BaseStyles, ThemeProvider, SSRProvider, ActionMenu} from '..' -import userEvent from '@testing-library/user-event' function SimpleActionList(): JSX.Element { return ( From 47f8cc9980504d5845b0786bcb7b2fb48c3360d2 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Mon, 11 Sep 2023 07:45:37 +1000 Subject: [PATCH 7/9] import useId from hooks not react --- src/ActionList/List.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ActionList/List.tsx b/src/ActionList/List.tsx index 7815407e206..ba2178d3efa 100644 --- a/src/ActionList/List.tsx +++ b/src/ActionList/List.tsx @@ -1,4 +1,4 @@ -import React, {useId} from 'react' +import React from 'react' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import styled from 'styled-components' import sx, {SxProp, merge} from '../sx' @@ -7,6 +7,7 @@ import {ActionListContainerContext} from './ActionListContainerContext' import {defaultSxProp} from '../utils/defaultSxProp' import {useSlots} from '../hooks/useSlots' import {Heading} from './Heading' +import {useId} from '../hooks/useId' export type ActionListProps = React.PropsWithChildren<{ /** From 32bb4404e311504027203a11b5799a13f6725408 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Mon, 11 Sep 2023 08:04:41 +1000 Subject: [PATCH 8/9] snaps --- .../__snapshots__/NavList.test.tsx.snap | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap index b50e0e3e252..7253cc36ff8 100644 --- a/src/NavList/__snapshots__/NavList.test.tsx.snap +++ b/src/NavList/__snapshots__/NavList.test.tsx.snap @@ -304,7 +304,7 @@ exports[`NavList renders a simple list 1`] = ` > Home @@ -326,7 +326,7 @@ exports[`NavList renders a simple list 1`] = ` class="c1 c6" > About @@ -348,7 +348,7 @@ exports[`NavList renders a simple list 1`] = ` class="c1 c6" > Contact @@ -718,13 +718,13 @@ exports[`NavList renders with groups 1`] = ` role="presentation" > Overview