From 50340c04087e56a4c2efa3d002d01a1d55a0da3b Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 3 Nov 2023 13:13:28 +1000 Subject: [PATCH 1/7] add group heading --- .../ActionList.features.stories.tsx | 140 ++++++++++++++++++ src/ActionList/Group.tsx | 81 ++++++++-- src/ActionList/index.ts | 5 +- 3 files changed, 210 insertions(+), 16 deletions(-) diff --git a/src/ActionList/ActionList.features.stories.tsx b/src/ActionList/ActionList.features.stories.tsx index fe3527f79d3..07bb8ef02e8 100644 --- a/src/ActionList/ActionList.features.stories.tsx +++ b/src/ActionList/ActionList.features.stories.tsx @@ -96,6 +96,146 @@ export const WithVisualListHeading = () => ( ) +export const ListWithNewGroupHeadingAPI = () => ( + + List Heading + + Group 1 Heading + {}}> + + + + app/assets/modules + + {}}> + + + + src/react/components + + {}}> + + + + memex/shared-ui/components + + {}}> + + + + views/assets/modules + + + + + Group 2 Heading + {}}> + + + + Owner + + {}}> + + + + Symbol + + {}}> + + + + Exclude archived + + + +) + +export const MenuWithNewGroupHeadingAPI = () => { + const [assignees, setAssignees] = React.useState(users.slice(0, 1)) + + const toggleAssignee = (assignee: (typeof users)[number]) => { + const assigneeIndex = assignees.findIndex(a => a.login === assignee.login) + + if (assigneeIndex === -1) setAssignees([...assignees, assignee]) + else setAssignees(assignees.filter((_, index) => index !== assigneeIndex)) + } + + return ( + + + Everyone + {users.slice(2).map(user => ( + assignee.login === user.login))} + aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))} + onSelect={() => toggleAssignee(user)} + > + + + + {user.login} + {user.name} + + ))} + + + ) +} + +export const ListboxWithNewGroupHeadingAPI = () => { + const [assignees, setAssignees] = React.useState(users.slice(0, 1)) + + const toggleAssignee = (assignee: (typeof users)[number]) => { + const assigneeIndex = assignees.findIndex(a => a.login === assignee.login) + + if (assigneeIndex === -1) setAssignees([...assignees, assignee]) + else setAssignees(assignees.filter((_, index) => index !== assigneeIndex)) + } + + return ( + + + Everyone + {users.slice(2).map(user => ( + assignee.login === user.login))} + aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))} + onSelect={() => toggleAssignee(user)} + > + + + + {user.login} + {user.name} + + ))} + + + Review Requested + {users.slice(2).map(user => ( + assignee.login === user.login))} + aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))} + onSelect={() => toggleAssignee(user)} + > + + + + {user.login} + {user.name} + + ))} + + + ) +} export const WithCustomHeading = () => ( <> diff --git a/src/ActionList/Group.tsx b/src/ActionList/Group.tsx index f6332760c1f..97632cb1172 100644 --- a/src/ActionList/Group.tsx +++ b/src/ActionList/Group.tsx @@ -1,9 +1,13 @@ import React from 'react' import {useId} from '../hooks/useId' import Box from '../Box' -import {SxProp} from '../sx' +import {SxProp, BetterSystemStyleObject, merge} from '../sx' import {ListContext, ActionListProps} from './List' import {AriaRole} from '../utils/types' +import {default as Heading} from '../Heading' +import {useSlots} from '../hooks/useSlots' +import {defaultSxProp} from '../utils/defaultSxProp' +import {warning} from '../utils/warning' export type ActionListGroupProps = { /** @@ -44,9 +48,15 @@ export const Group: React.FC> = ({ sx = {}, ...props }) => { - const labelId = useId() + const id = useId() const {role: listRole} = React.useContext(ListContext) + const [slots, childrenWithoutSlots] = useSlots(props.children, { + groupHeading: GroupHeading, + }) + + const headingId = slots.groupHeading ? slots.groupHeading.props.id ?? id : title ? id : undefined + return ( > = ({ }} {...props} > - {title &&
} + {(title || slots.groupHeading) && ( + + {slots.groupHeading ? slots.groupHeading.props.children : null} + + )} - {props.children} + {slots.groupHeading ? childrenWithoutSlots : props.children} ) } -export type HeaderProps = Pick & { - labelId: string -} +export type GroupHeadingProps = Pick & { + as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' + labelId?: string + id?: string +} & SxProp /** * Displays the name and description of a `Group`. * - * For visual presentation only. It's hidden from screen readers. + * For visual presentation only. */ -const Header: React.FC> = ({variant, title, auxiliaryText, labelId, ...props}) => { - const {variant: listVariant} = React.useContext(ListContext) +export const GroupHeading: React.FC> = ({ + as, + variant, + title, + auxiliaryText, + labelId, + children, + sx = defaultSxProp, + ...props +}) => { + const {variant: listVariant, role: listRole} = React.useContext(ListContext) + // for list role, the headings are proper heading tags, for menu and listbox, they are just representational and divs + warning( + listRole === undefined && children !== null && as === undefined, + `You are setting a heading for a list, that requires a heading level. Please use ` as ` prop to set the proper heading level.`, + ) const styles = { paddingY: '6px', @@ -102,9 +139,23 @@ const Header: React.FC> = ({variant, title, } return ( - + <> + {listRole ? ( + + ) : ( + (styles, sx)} + {...props} + > + {title ?? children} + + )} + ) } diff --git a/src/ActionList/index.ts b/src/ActionList/index.ts index 202a96a1a4b..0bc89ae5986 100644 --- a/src/ActionList/index.ts +++ b/src/ActionList/index.ts @@ -1,5 +1,5 @@ import {List} from './List' -import {Group} from './Group' +import {Group, GroupHeading} from './Group' import {Item} from './Item' import {LinkItem} from './LinkItem' import {Divider} from './Divider' @@ -43,4 +43,7 @@ export const ActionList = Object.assign(List, { /** Heading for an `ActionList`. */ Heading, + + /** Heading for `ActionList.Group` */ + GroupHeading, }) From f8f60f0a3d50ad67b18f22f41e1c334fbe4f39da Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 3 Nov 2023 16:05:48 +1000 Subject: [PATCH 2/7] update snapshots --- src/ActionList/Group.tsx | 2 +- .../__snapshots__/NavList.test.tsx.snap | 49 ++++++++----------- 2 files changed, 22 insertions(+), 29 deletions(-) diff --git a/src/ActionList/Group.tsx b/src/ActionList/Group.tsx index 97632cb1172..803a4176ef8 100644 --- a/src/ActionList/Group.tsx +++ b/src/ActionList/Group.tsx @@ -119,7 +119,7 @@ export const GroupHeading: React.FC> // for list role, the headings are proper heading tags, for menu and listbox, they are just representational and divs warning( listRole === undefined && children !== null && as === undefined, - `You are setting a heading for a list, that requires a heading level. Please use ` as ` prop to set the proper heading level.`, + `You are setting a heading for a list, that requires a heading level. Please use 'as' prop to set a proper heading level.`, ) const styles = { diff --git a/src/NavList/__snapshots__/NavList.test.tsx.snap b/src/NavList/__snapshots__/NavList.test.tsx.snap index d90f6f1c4c1..eebaf1c1e46 100644 --- a/src/NavList/__snapshots__/NavList.test.tsx.snap +++ b/src/NavList/__snapshots__/NavList.test.tsx.snap @@ -399,16 +399,6 @@ exports[`NavList renders with groups 1`] = ` margin-top: 8px; } -.c3 { - padding-top: 6px; - padding-bottom: 6px; - padding-left: 16px; - padding-right: 16px; - font-size: 12px; - font-weight: 600; - color: #656d76; -} - .c4 { padding-inline-start: 0; } @@ -436,6 +426,19 @@ exports[`NavList renders with groups 1`] = ` font-weight: 400; } +.c3 { + font-weight: 600; + font-size: 32px; + margin: 0; + padding-top: 6px; + padding-bottom: 6px; + padding-left: 16px; + padding-right: 16px; + font-size: 12px; + font-weight: 600; + color: #656d76; +} + .c0 { margin: 0; padding-inline-start: 0; @@ -723,17 +726,12 @@ exports[`NavList renders with groups 1`] = `
  • - + Overview +
      - + Components +
        Date: Mon, 6 Nov 2023 15:05:17 +1000 Subject: [PATCH 3/7] add tests, clean up stuff --- .../ActionList.features.stories.tsx | 2 +- src/ActionList/ActionList.test.tsx | 97 +++++++++++++++++++ src/ActionList/Group.tsx | 53 ++++++---- 3 files changed, 131 insertions(+), 21 deletions(-) diff --git a/src/ActionList/ActionList.features.stories.tsx b/src/ActionList/ActionList.features.stories.tsx index 07bb8ef02e8..ba3898791f3 100644 --- a/src/ActionList/ActionList.features.stories.tsx +++ b/src/ActionList/ActionList.features.stories.tsx @@ -99,7 +99,7 @@ export const WithVisualListHeading = () => ( export const ListWithNewGroupHeadingAPI = () => ( List Heading - + Group 1 Heading {}}> diff --git a/src/ActionList/ActionList.test.tsx b/src/ActionList/ActionList.test.tsx index cd2cf3219b9..7ec20887ea2 100644 --- a/src/ActionList/ActionList.test.tsx +++ b/src/ActionList/ActionList.test.tsx @@ -5,6 +5,7 @@ import theme from '../theme' import {ActionList} from '.' import {behavesAsComponent, checkExports} from '../utils/testing' import {BaseStyles, ThemeProvider, SSRProvider, ActionMenu} from '..' +import data from '../drafts/SelectPanel2/mock-data' function SimpleActionList(): JSX.Element { return ( @@ -215,4 +216,100 @@ describe('ActionList', () => { expect(spy).toHaveBeenCalled() spy.mockRestore() }) + it('should render the ActionList.GroupHeading component as a heading with the given heading level', async () => { + const container = HTMLRender( + + Heading + + Group Heading + + , + ) + const heading = container.getByRole('heading', {level: 2}) + expect(heading).toBeInTheDocument() + expect(heading).toHaveTextContent('Group Heading') + }) + it('should throw a warning if ActionList.Group is used without as prop when no role is specified (for list role)', async () => { + const spy = jest.spyOn(console, 'warn').mockImplementationOnce(() => {}) + + HTMLRender( + + Heading + + Group Heading + + , + ) + expect(spy).toHaveBeenCalledTimes(1) + spy.mockRestore() + }) + it('should render the ActionList.GroupHeading component as a span (not a heading tag) when role is specified as listbox', async () => { + const container = HTMLRender( + + Heading + + Group Heading + + , + ) + const label = container.getByText('Group Heading') + expect(label).toBeInTheDocument() + expect(label.tagName).toEqual('SPAN') + }) + it('should render the ActionList.GroupHeading component as a span with role="presentation" and aria-hidden="true" when role is specified as listbox', async () => { + const container = HTMLRender( + + Heading + + Group Heading + + , + ) + const label = container.getByText('Group Heading') + const wrapper = label.parentElement + expect(wrapper).toHaveAttribute('role', 'presentation') + expect(wrapper).toHaveAttribute('aria-hidden', 'true') + }) + it('should label the list with the group heading id', async () => { + const {container, getByText} = HTMLRender( + + Heading + + Group Heading + Item + + , + ) + const list = container.querySelector(`li[data-test-id='ActionList.Group'] > ul`) + const heading = getByText('Group Heading') + expect(list).toHaveAttribute('aria-labelledby', heading.id) + }) + it('should NOT label the list with the group heading id when role is specified', async () => { + const {container, getByText} = HTMLRender( + + Heading + + Group Heading + Item + + , + ) + const list = container.querySelector(`li[data-test-id='ActionList.Group'] > ul`) + const heading = getByText('Group Heading') + expect(list).not.toHaveAttribute('aria-labelledby', heading.id) + }) + it('should label the list using aria-label when role is specified', async () => { + const {container, getByText} = HTMLRender( + + Heading + + Group Heading + Item + + , + ) + const list = container.querySelector(`li[data-test-id='ActionList.Group'] > ul`) + const heading = getByText('Group Heading') + expect(list).toHaveAttribute('aria-label', heading.textContent) + }) }) diff --git a/src/ActionList/Group.tsx b/src/ActionList/Group.tsx index 803a4176ef8..5f92f2f4629 100644 --- a/src/ActionList/Group.tsx +++ b/src/ActionList/Group.tsx @@ -5,6 +5,7 @@ import {SxProp, BetterSystemStyleObject, merge} from '../sx' import {ListContext, ActionListProps} from './List' import {AriaRole} from '../utils/types' import {default as Heading} from '../Heading' +import type {ActionListHeadingProps} from './Heading' import {useSlots} from '../hooks/useSlots' import {defaultSxProp} from '../utils/defaultSxProp' import {warning} from '../utils/warning' @@ -55,7 +56,17 @@ export const Group: React.FC> = ({ groupHeading: GroupHeading, }) - const headingId = slots.groupHeading ? slots.groupHeading.props.id ?? id : title ? id : undefined + let groupHeadingId = undefined + + // ActionList.GroupHeading + if (slots.groupHeading) { + // If there is an id prop passed in the ActionList.GroupHeading, use it otherwise use the generated id. + groupHeadingId = slots.groupHeading.props.id ?? id + } + // Supports the deprecated `title` prop + if (title) { + groupHeadingId = id + } return ( > = ({ }} {...props} > - {(title || slots.groupHeading) && ( + {(slots.groupHeading || title) && ( {slots.groupHeading ? slots.groupHeading.props.children : null} @@ -83,7 +94,10 @@ export const Group: React.FC> = ({ ) -export const ListWithNewGroupHeadingAPI = () => ( - - List Heading - - Group 1 Heading - {}}> - - - - app/assets/modules - - {}}> - - - - src/react/components - - {}}> - - - - memex/shared-ui/components - - {}}> - - - - views/assets/modules - - - - - Group 2 Heading - {}}> - - - - Owner - - {}}> - - - - Symbol - - {}}> - - - - Exclude archived - - - -) - -export const MenuWithNewGroupHeadingAPI = () => { - const [assignees, setAssignees] = React.useState(users.slice(0, 1)) - - const toggleAssignee = (assignee: (typeof users)[number]) => { - const assigneeIndex = assignees.findIndex(a => a.login === assignee.login) - - if (assigneeIndex === -1) setAssignees([...assignees, assignee]) - else setAssignees(assignees.filter((_, index) => index !== assigneeIndex)) - } - - return ( - - - Everyone - {users.slice(2).map(user => ( - assignee.login === user.login))} - aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))} - onSelect={() => toggleAssignee(user)} - > - - - - {user.login} - {user.name} - - ))} - - - ) -} - -export const ListboxWithNewGroupHeadingAPI = () => { - const [assignees, setAssignees] = React.useState(users.slice(0, 1)) - - const toggleAssignee = (assignee: (typeof users)[number]) => { - const assigneeIndex = assignees.findIndex(a => a.login === assignee.login) - - if (assigneeIndex === -1) setAssignees([...assignees, assignee]) - else setAssignees(assignees.filter((_, index) => index !== assigneeIndex)) - } - - return ( - - - Everyone - {users.slice(2).map(user => ( - assignee.login === user.login))} - aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))} - onSelect={() => toggleAssignee(user)} - > - - - - {user.login} - {user.name} - - ))} - - - Review Requested - {users.slice(2).map(user => ( - assignee.login === user.login))} - aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))} - onSelect={() => toggleAssignee(user)} - > - - - - {user.login} - {user.name} - - ))} - - - ) -} export const WithCustomHeading = () => ( <> diff --git a/src/ActionList/Group.tsx b/src/ActionList/Group.tsx index 8d499bb6e27..97a39107306 100644 --- a/src/ActionList/Group.tsx +++ b/src/ActionList/Group.tsx @@ -83,11 +83,10 @@ export const Group: React.FC> = ({ {...props} > - {slots.groupHeading || title ? ( - - {slots.groupHeading ? slots.groupHeading.props.children : null} - + {title && !slots.groupHeading ? ( + ) : null} + {!title && slots.groupHeading ? React.cloneElement(slots.groupHeading) : null}