Skip to content
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.

Sign up for GitHub

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

ActionList.GroupHeading roll out improvements #4395

Merged
merged 9 commits into from
Mar 27, 2024
Merged

ActionList.GroupHeading roll out improvements #4395

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cbcd7b5
ActionList.GroupHeading roll out improvements
broccolinisoup Mar 15, 2024
86823a3
fix tests
broccolinisoup Mar 15, 2024
f94e8d2
add a deprecated TS notice
broccolinisoup Mar 18, 2024
9b21aa0
Create chilly-buckets-kick.md
broccolinisoup Mar 21, 2024
5dc9a53
Merge branch 'main' into actionlist-group-heading-inprovements
broccolinisoup Mar 27, 2024
8eabc6e
messed up the conflict
broccolinisoup Mar 27, 2024
4f154cf
code review comment address
broccolinisoup Mar 27, 2024
114cac6
fix test and add another case
broccolinisoup Mar 27, 2024
e6a19bc
add an underscore to the internal bc title
broccolinisoup Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/chilly-buckets-kick.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@primer/react": minor
---
ActionList.Group: deprecate `title` prop - please use `ActionList.GroupHeading` instead
ActionList.GroupHeading: update the warning to be an error if there is no explict `as` prop for list `role` action lists.
ActionList.GroupHeading: There shouldn't be an `as` prop on `ActionList.GroupHeading` for `listbox` or `menu` role action lists. console.error if there is one
6 changes: 4 additions & 2 deletions packages/react/src/ActionList/ActionList.features.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ export const SimpleList = () => (
export const WithVisualListHeading = () => (
<ActionList>
<ActionList.Heading as="h2">Filter by</ActionList.Heading>
<ActionList.Group title="Path">
<ActionList.Group>
<ActionList.GroupHeading as="h4">Repositories</ActionList.GroupHeading>
<ActionList.Item onClick={() => {}}>
<ActionList.LeadingVisual>
<FileDirectoryIcon />
Expand All @@ -73,7 +74,8 @@ export const WithVisualListHeading = () => (
</ActionList.Item>
</ActionList.Group>

<ActionList.Group title="Advanced">
<ActionList.Group>
<ActionList.GroupHeading as="h4">Advanced</ActionList.GroupHeading>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, do we have a story that still uses the old API? Just to keep it around for regressions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I added them as dev stories https://github.com/primer/react/blob/main/packages/react/src/ActionList/ActionList.dev.stories.tsx and playwright snapshots them too to catch any regression 😈

<ActionList.Item onClick={() => {}}>
<ActionList.LeadingVisual>
<PlusCircleIcon />
Expand Down
6 changes: 3 additions & 3 deletions packages/react/src/ActionList/ActionList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,8 @@ describe('ActionList', () => {
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(() => {})
it('should throw an error if ActionList.GroupHeading is used without an `as` prop when no role is specified (for list role)', async () => {
Copy link
Member Author

@broccolinisoup broccolinisoup Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails https://github.com/primer/react/actions/runs/8368326991/job/22912262706?pr=4395 and I am so confused why - it is throwing an error and I am expecting the error in the test 🤷🏻‍♀️ Am I missing something obvious?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have to explicitly expect it to throw instead of spy

example in primer: https://github.com/primer/react/blob/main/packages/react/src/__tests__/Autocomplete.test.tsx#L444
stack overflow: https://stackoverflow.com/a/46155381

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, right! it did the trick, thanks so much 🙌🏻

const spy = jest.spyOn(console, 'error').mockImplementationOnce(() => {})

HTMLRender(
<ActionList>
Expand All @@ -273,7 +273,7 @@ describe('ActionList', () => {
</ActionList.Group>
</ActionList>,
)
expect(spy).toHaveBeenCalledTimes(1)
expect(spy).toHaveBeenCalled()
spy.mockRestore()
})
it('should render the ActionList.GroupHeading component as a span (not a heading tag) when role is specified as listbox', async () => {
Expand Down
36 changes: 26 additions & 10 deletions packages/react/src/ActionList/Group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ 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'
import {invariant} from '../utils/invariant'

export type ActionListGroupProps = {
/**
Expand All @@ -20,7 +20,7 @@ export type ActionListGroupProps = {
*/
variant?: 'subtle' | 'filled'
/**
* Primary text which names a `Group`.
* @deprecated (Use `ActionList.GroupHeading` instead. i.e. <ActionList.Group title="Group title"> → <ActionList.GroupHeading>Group title</ActionList.GroupHeading>)
*/
title?: string
/**
Expand Down Expand Up @@ -85,8 +85,10 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
>
<GroupContext.Provider value={{selectionVariant, groupHeadingId}}>
{title && !slots.groupHeading ? (
<GroupHeading title={title} variant={variant} auxiliaryText={auxiliaryText} />
// Escape hatch: supports old API <ActionList.Group title="group title"> in a non breaking way
<GroupHeading variant={variant} auxiliaryText={auxiliaryText} unsafeTitle={title} />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure that the invariant check doesn't go through on the old API so that it doesn't break any usecases in downstream repos, I kept the ActionList.GroupHeading sub component's API flexible. Especially, to distinguish where the group heading is coming from i.e. old api vs new api.

If it comes from old api (i.e. if it has title prop), I render the heading like

<GroupHeading variant={variant} auxiliaryText={auxiliaryText} title={title} />

if it uses the new api (if children exists),

<GroupHeading variant={variant} auxiliaryText={auxiliaryText} >{title}</GroupHeading>

Copy link
Member

@siddharthkp siddharthkp Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional name idea, instead of unsafeTitle: internalBackwardCompatibleTitle

) : null}
{/* Supports new API ActionList.GroupHeading */}
{!title && slots.groupHeading ? React.cloneElement(slots.groupHeading) : null}
<Box
as="ul"
Expand All @@ -105,11 +107,12 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
)
}

export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'title' | 'auxiliaryText'> &
export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'auxiliaryText'> &
Omit<ActionListHeadingProps, 'as'> &
SxProp &
React.HTMLAttributes<HTMLElement> & {
as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
unsafeTitle?: string
}

/**
Expand All @@ -123,7 +126,8 @@ export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'title' |
export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>> = ({
as,
variant,
title,
// We are not recommending this prop to be used, it should only be used internally for incremental rollout.
unsafeTitle,
auxiliaryText,
children,
sx = defaultSxProp,
Expand All @@ -132,11 +136,21 @@ export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>>
const {variant: listVariant, role: listRole} = React.useContext(ListContext)
const {groupHeadingId} = React.useContext(GroupContext)
// for list role, the headings are proper heading tags, for menu and listbox, they are just representational and divs
warning(
listRole === undefined && children !== undefined && as === undefined,
const missingAsForList = (listRole === undefined || listRole === 'list') && children !== undefined && as === undefined
const unnecessaryAsForListboxOrMenu =
listRole !== undefined && listRole !== 'list' && children !== undefined && as !== undefined
invariant(
// 'as' prop is required for list roles. <GroupHeading as="h2">...</GroupHeading>
!missingAsForList,
`You are setting a heading for a list, that requires a heading level. Please use 'as' prop to set a proper heading level.`,
)

invariant(
// 'as' prop on listbox or menu roles are not needed since they are rendered as divs and they could be misleading.
!unnecessaryAsForListboxOrMenu,
`Looks like you are trying to set a heading level to a ${listRole} role. Group headings for ${listRole} type action lists are for representational purposes, and rendered as divs. Therefore they don't need a heading level.`,
)

const styles = {
paddingY: '6px',
paddingX: listVariant === 'full' ? 2 : 3,
Expand All @@ -155,15 +169,17 @@ export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>>

return (
<>
{listRole ? (
{/* for listbox (SelectPanel) and menu (ActionMenu) roles, group titles are presentational. */}
{listRole && listRole !== 'list' ? (
<Box sx={styles} role="presentation" aria-hidden="true" {...props}>
<span id={groupHeadingId}>{title ?? children}</span>
<span id={groupHeadingId}>{unsafeTitle ?? children}</span>
{auxiliaryText && <span>{auxiliaryText}</span>}
</Box>
) : (
// for explicit (role="list" is passed as prop) and implicit list roles (ActionList ins rendered as list by default), group titles are proper heading tags.
<>
<Heading as={as || 'h3'} id={groupHeadingId} sx={merge<BetterSystemStyleObject>(styles, sx)} {...props}>
{title ?? children}
{unsafeTitle ?? children}
</Heading>
{auxiliaryText && <span>{auxiliaryText}</span>}
</>
Expand Down
Loading