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.

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

Adds NavList.GroupHeading component #5106

Merged
merged 10 commits into from
Oct 22, 2024
5 changes: 5 additions & 0 deletions .changeset/twelve-kings-confess.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Adds NavList.GroupHeading component that can be used instead of the ActionList.Group 'title' prop if you need to render something besides a string
4 changes: 2 additions & 2 deletions packages/react/src/ActionList/Group.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
)
}

export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'auxiliaryText'> &
export type ActionListGroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'auxiliaryText'> &
Omit<ActionListHeadingProps, 'as'> &
SxProp &
React.HTMLAttributes<HTMLElement> & {
Expand All @@ -123,7 +123,7 @@ export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'auxiliar
* hidden from the accessibility tree due to the limitation of listbox children. https://w3c.github.io/aria/#listbox
* groups under menu or listbox are labelled by `aria-label`
*/
export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>> = ({
export const GroupHeading: React.FC<React.PropsWithChildren<ActionListGroupHeadingProps>> = ({
as,
variant,
// We are not recommending this prop to be used, it should only be used internally for incremental rollout.
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/ActionList/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {LeadingVisual, TrailingVisual} from './Visuals'
import {Heading} from './Heading'

export type {ActionListProps} from './shared'
export type {ActionListGroupProps} from './Group'
export type {ActionListGroupProps, ActionListGroupHeadingProps} from './Group'
export type {ActionListItemProps} from './shared'
export type {ActionListLinkItemProps} from './LinkItem'
export type {ActionListDividerProps} from './Divider'
Expand Down
18 changes: 18 additions & 0 deletions packages/react/src/NavList/NavList.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,24 @@
},
{
"name": "NavList.Group",
"props": [
{
"name": "title",
"type": "string",
"description": "The text that gets rendered as the group's heading. Alternatively, you can pass the `NavList.GroupHeading` component as a child of `NavList.Group`.\n If both `title` and `NavList.GroupHeading` are passed, `NavList.GroupHeading` will be rendered as the heading."
},
{
"name": "sx",
"type": "SystemStyleObject"
},
{
"name": "ref",
"type": "React.RefObject<HTMLElement>"
}
]
},
{
"name": "NavList.GroupHeading",
"props": [
{
"name": "sx",
Expand Down
28 changes: 28 additions & 0 deletions packages/react/src/NavList/NavList.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,34 @@ export const WithGroup: StoryFn = () => (
</PageLayout>
)

export const WithGroupHeadingLinks: StoryFn = () => (
<PageLayout>
<PageLayout.Pane position="start">
<NavList>
<NavList.Group>
<NavList.GroupHeading>
<a href="#group-1">Group 1</a>
</NavList.GroupHeading>
<NavList.Item aria-current="true" href="#">
Item 1A
</NavList.Item>
<NavList.Item href="#">Item 1B</NavList.Item>
<NavList.Item href="#">Item 1C</NavList.Item>
</NavList.Group>
<NavList.Group>
<NavList.GroupHeading>
<a href="#group-2">Group 2</a>
</NavList.GroupHeading>
<NavList.Item href="#">Item 2A</NavList.Item>
<NavList.Item href="#">Item 2B</NavList.Item>
<NavList.Item href="#">Item 2C</NavList.Item>
</NavList.Group>
</NavList>
</PageLayout.Pane>
<PageLayout.Content></PageLayout.Content>
</PageLayout>
)

export const WithTrailingAction = () => {
return (
<PageLayout>
Expand Down
23 changes: 23 additions & 0 deletions packages/react/src/NavList/NavList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,29 @@ describe('NavList', () => {
expect(container).toMatchSnapshot()
})

it('only shows NavList.GroupHeading when NavList.Group `title` prop is passed AND NavList.GroupHeading is a child', () => {
const {getByText} = render(
<ThemeProvider>
<NavList>
<NavList.Group title="Overview">
<NavList.GroupHeading>Group heading</NavList.GroupHeading>
<NavList.Item href="/getting-started" aria-current="page">
Getting started
</NavList.Item>
</NavList.Group>
<NavList.Group title="Components">
<NavList.Item href="/Avatar">Avatar</NavList.Item>
</NavList.Group>
</NavList>
</ThemeProvider>,
)
const groupHeading = getByText('Group heading')
const groupTitle = getByText('Overview')

expect(groupHeading).toBeVisible()
expect(groupTitle).not.toBeVisible()
})

it('supports TrailingAction', async () => {
const {getByRole} = render(
<NavList>
Expand Down
44 changes: 42 additions & 2 deletions packages/react/src/NavList/NavList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {
ActionListDividerProps,
ActionListLeadingVisualProps,
ActionListTrailingVisualProps,
ActionListGroupHeadingProps,
} from '../ActionList'
import {ActionList} from '../ActionList'
import {ActionListContainerContext} from '../ActionList/ActionListContainerContext'
Expand Down Expand Up @@ -279,9 +280,21 @@ const Group: React.FC<NavListGroupProps> = ({title, children, sx: sxProp = defau
<>
{/* Hide divider if the group is the first item in the list */}
<ActionList.Divider sx={{'&:first-child': {display: 'none'}}} />
<ActionList.Group {...props} sx={sxProp}>
<ActionList.Group
{...props}
// If somebody tries to pass the `title` prop AND a `NavList.GroupHeading` as a child, hide the `ActionList.GroupHeading`
sx={merge<SxProp['sx']>(sxProp, {
':has([data-component="NavList.GroupHeading"]):has([data-component="ActionList.GroupHeading"])': {
'[data-component="ActionList.GroupHeading"]': {display: 'none'},
},
})}
>
{/* Setting up the default value for the heading level. TODO: API update to give flexibility to NavList.Group title's heading level */}
{title ? <ActionList.GroupHeading as="h3">{title}</ActionList.GroupHeading> : null}
{title ? (
<ActionList.GroupHeading as="h3" data-component="ActionList.GroupHeading">
{title}
</ActionList.GroupHeading>
) : null}
{children}
</ActionList.Group>
</>
Expand All @@ -290,6 +303,32 @@ const Group: React.FC<NavListGroupProps> = ({title, children, sx: sxProp = defau

Group.displayName = 'NavList.Group'

export type NavListGroupHeadingProps = ActionListGroupHeadingProps

/**
* This is an alternative to the `title` prop on `NavList.Group`.
* It was primarily added to allow links in group headings.
*/
const GroupHeading: React.FC<NavListGroupHeadingProps> = ({as = 'h3', sx: sxProp = defaultSxProp, ...rest}) => {
return (
<ActionList.GroupHeading
as={as}
sx={merge<SxProp['sx']>(
{
'> a {': {
color: 'var(--fgColor-default)',
textDecoration: 'inherit',
':hover': {textDecoration: 'underline'},
},
},
sxProp,
)}
Comment on lines +316 to +325
Copy link
Member

Choose a reason for hiding this comment

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

Do we still want to use sx here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to use regular CSS for component styling in PRC yet?

Copy link
Member

Choose a reason for hiding this comment

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

If you're adding new styles I think it's safe to go directly to CSS modules. The risk is entirely around the fact that there could be other styles in dotcom relying on the existing sx styling architecture.

data-component="NavList.GroupHeading"
{...rest}
/>
)
}

// ----------------------------------------------------------------------------
// Export

Expand All @@ -301,4 +340,5 @@ export const NavList = Object.assign(Root, {
TrailingAction,
Divider,
Group,
GroupHeading,
})
50 changes: 28 additions & 22 deletions packages/react/src/NavList/__snapshots__/NavList.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,10 @@ exports[`NavList renders with groups 1`] = `
margin-top: 8px;
}

.c2:has([data-component="NavList.GroupHeading"]):has([data-component="ActionList.GroupHeading"]) [data-component="ActionList.GroupHeading"] {
display: none;
}

.c3 {
padding-top: 6px;
padding-bottom: 6px;
Expand Down Expand Up @@ -872,6 +876,7 @@ exports[`NavList renders with groups 1`] = `
>
<h3
class="ActionListGroupHeading Heading"
data-component="ActionList.GroupHeading"
id=":r7:"
>
Overview
Expand Down Expand Up @@ -920,6 +925,7 @@ exports[`NavList renders with groups 1`] = `
>
<h3
class="ActionListGroupHeading Heading"
data-component="ActionList.GroupHeading"
id=":r9:"
>
Components
Expand Down Expand Up @@ -1389,15 +1395,15 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav
class="c0"
>
<li
aria-labelledby=":r26:"
aria-labelledby=":r2c:"
class="c1 c2"
>
<button
aria-controls=":r27:"
aria-controls=":r2d:"
aria-expanded="true"
aria-labelledby=":r26:--label :r26:--trailing-visual "
aria-labelledby=":r2c:--label :r2c:--trailing-visual "
class="c3 c4"
id=":r26:"
id=":r2c:"
tabindex="0"
>
<div
Expand All @@ -1409,13 +1415,13 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav
>
<span
class="c1 c7"
id=":r26:--label"
id=":r2c:--label"
>
Item
</span>
<span
class="c1 c8"
id=":r26:--trailing-visual"
id=":r2c:--trailing-visual"
>
<svg
aria-hidden="true"
Expand All @@ -1437,19 +1443,19 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav
</button>
<div>
<ul
aria-labelledby=":r26:"
aria-labelledby=":r2c:"
class="c1 c10"
id=":r27:"
id=":r2d:"
>
<li
class="c3 c11"
>
<a
aria-current="page"
aria-labelledby=":r29:--label "
aria-labelledby=":r2f:--label "
class="c12 c13"
href="#"
id=":r29:"
id=":r2f:"
tabindex="0"
>
<div
Expand All @@ -1458,7 +1464,7 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav
>
<span
class="c1 c14"
id=":r29:--label"
id=":r2f:--label"
>
Sub Item
</span>
Expand Down Expand Up @@ -1923,15 +1929,15 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t
class="c0"
>
<li
aria-labelledby=":r20:"
aria-labelledby=":r26:"
class="c1 c2"
>
<button
aria-controls=":r21:"
aria-controls=":r27:"
aria-expanded="false"
aria-labelledby=":r20:--label :r20:--trailing-visual "
aria-labelledby=":r26:--label :r26:--trailing-visual "
class="c3 c4"
id=":r20:"
id=":r26:"
tabindex="0"
>
<div
Expand All @@ -1943,13 +1949,13 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t
>
<span
class="c1 c7"
id=":r20:--label"
id=":r26:--label"
>
Item
</span>
<span
class="c1 c8"
id=":r20:--trailing-visual"
id=":r26:--trailing-visual"
>
<svg
aria-hidden="true"
Expand All @@ -1971,19 +1977,19 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t
</button>
<div>
<ul
aria-labelledby=":r20:"
aria-labelledby=":r26:"
class="c1 c10"
id=":r21:"
id=":r27:"
>
<li
class="c3 c11"
>
<a
aria-current="page"
aria-labelledby=":r23:--label "
aria-labelledby=":r29:--label "
class="c12 c13"
href="#"
id=":r23:"
id=":r29:"
tabindex="0"
>
<div
Expand All @@ -1992,7 +1998,7 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t
>
<span
class="c1 c7"
id=":r23:--label"
id=":r29:--label"
>
Sub Item
</span>
Expand Down
Loading