diff --git a/.changeset/fuzzy-lists-count.md b/.changeset/fuzzy-lists-count.md new file mode 100644 index 00000000000..bd79667042c --- /dev/null +++ b/.changeset/fuzzy-lists-count.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +NavList: Exclude group headings from navigation list item counts \ No newline at end of file diff --git a/packages/react/src/ActionList/Group.module.css b/packages/react/src/ActionList/Group.module.css index 0273e901e77..8862185c5ed 100644 --- a/packages/react/src/ActionList/Group.module.css +++ b/packages/react/src/ActionList/Group.module.css @@ -3,15 +3,6 @@ &:not(:first-child) { margin-block-start: var(--base-size-8); - - /* If somebody tries to pass the `title` prop AND a `NavList.GroupHeading` as a child, hide the `ActionList.GroupHeading */ - /* stylelint-disable-next-line selector-max-specificity, selector-pseudo-class-disallowed-list -- scoped to CSS Module, audited (github/github-ui#17224) */ - &:has(.GroupHeadingWrap + ul > .GroupHeadingWrap) { - /* stylelint-disable-next-line selector-max-specificity */ - & > .GroupHeadingWrap { - display: none; - } - } } } diff --git a/packages/react/src/NavList/NavList.test.tsx b/packages/react/src/NavList/NavList.test.tsx index e3121b2e6d6..f65e20151d9 100644 --- a/packages/react/src/NavList/NavList.test.tsx +++ b/packages/react/src/NavList/NavList.test.tsx @@ -2,10 +2,11 @@ import {describe, it, expect, vi} from 'vitest' import {render, fireEvent, act} from '@testing-library/react' import React from 'react' import {renderToStaticMarkup} from 'react-dom/server' -import {NavList} from './NavList' +import {NavList, type NavListGroupHeadingProps} from './NavList' import {ReactRouterLikeLink} from '../Pagination/mocks/ReactRouterLink' import {implementsClassName} from '../utils/testing' import {FeatureFlags} from '../FeatureFlags' +import {asSlot} from '../utils/as-slot' type NextJSLinkProps = {href: string; children: React.ReactNode} @@ -754,6 +755,93 @@ describe('NavList.ShowMoreItem with pages', () => { }) describe('NavList.Group', () => { + it('renders the group heading outside of the nested list', () => { + const {container, getByRole} = render( + + + Project templates + Featured + Recent + Mine + + , + ) + + const group = container.querySelector('[data-testid="group"]') + const list = group?.querySelector(':scope > ul') + const heading = getByRole('heading', {level: 2, name: 'Project templates'}) + + expect(group).not.toBeNull() + expect(list).not.toBeNull() + expect(list?.children).toHaveLength(3) + expect(list).not.toContainElement(heading) + expect(heading.parentElement?.parentElement).toBe(group) + expect(list).toHaveAttribute('aria-labelledby', heading.id) + }) + + it('prefers a fragment-wrapped group heading over the title prop', () => { + const {container, getByRole, queryByText} = render( + + + <> + Project templates + + Featured + + , + ) + + const group = container.querySelector('[data-testid="group"]') + const list = container.querySelector('[data-testid="group"] > ul') + const heading = getByRole('heading', {level: 3, name: 'Project templates'}) + + expect(queryByText('Overview')).not.toBeInTheDocument() + expect(getByRole('heading')).toBe(heading) + expect(container.querySelectorAll(`#${CSS.escape(heading.id)}`)).toHaveLength(1) + expect(heading.parentElement?.parentElement).toBe(group) + expect(list).toHaveAttribute('aria-labelledby', heading.id) + expect(list?.children).toHaveLength(1) + }) + + it('prefers an explicit group heading over the title prop', () => { + const {getByRole, queryByText} = render( + + + Project templates + Featured + + , + ) + + expect(getByRole('heading', {level: 3, name: 'Project templates'})).toBeInTheDocument() + expect(queryByText('Overview')).not.toBeInTheDocument() + }) + + it('recognizes a group heading wrapper created with asSlot', () => { + const WrappedGroupHeading = asSlot( + ({children, ...props}: NavListGroupHeadingProps) => ( + {children} + ), + NavList.GroupHeading, + ) + const {container, getByRole, queryByText} = render( + + + Project templates + Featured + + , + ) + + const group = container.querySelector('[data-testid="group"]') + const list = group?.querySelector(':scope > ul') + const heading = getByRole('heading', {level: 3, name: 'Project templates'}) + + expect(queryByText('Overview')).not.toBeInTheDocument() + expect(heading.parentElement?.parentElement).toBe(group) + expect(list).toHaveAttribute('aria-labelledby', 'project-templates-heading') + }) + it('renders a divider before the group by default', () => { const {container} = render( diff --git a/packages/react/src/NavList/NavList.tsx b/packages/react/src/NavList/NavList.tsx index 9ff77d01548..6ca385849ef 100644 --- a/packages/react/src/NavList/NavList.tsx +++ b/packages/react/src/NavList/NavList.tsx @@ -22,6 +22,7 @@ import {fixedForwardRef, type PolymorphicProps} from '../utils/modern-polymorphi import HeadingComponent from '../Heading' import visuallyHiddenClasses from '../_VisuallyHidden.module.css' import type {FCWithSlotMarker} from '../utils/types/Slots' +import {asSlot} from '../utils/as-slot' type HeadingLevels = 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' @@ -366,8 +367,39 @@ export type NavListGroupProps = React.HTMLAttributes & { hideDivider?: boolean } +function flattenFragmentChildren(children: React.ReactNode): React.ReactNode[] { + const flattenedChildren: React.ReactNode[] = [] + // eslint-disable-next-line github/array-foreach -- React.Children.toArray changes element identity, which is needed to remove the slotted child from its fragment. + React.Children.forEach(children, child => { + if (React.isValidElement(child) && child.type === React.Fragment) { + flattenedChildren.push(...flattenFragmentChildren((child.props as {children?: React.ReactNode}).children)) + } else { + flattenedChildren.push(child) + } + }) + return flattenedChildren +} + +function removeChildFromFragments(children: React.ReactNode, childToRemove: React.ReactElement): React.ReactNode { + return React.Children.map(children, child => { + if (child === childToRemove) return null + if (React.isValidElement(child) && child.type === React.Fragment) { + return React.cloneElement( + child, + undefined, + removeChildFromFragments((child.props as {children?: React.ReactNode}).children, childToRemove), + ) + } + return child + }) +} + const Group: React.FC = ({title, children, hideDivider, ...props}) => { const headingLevel = React.useContext(NavListHeadingLevelContext) + const [slots] = useSlots(flattenFragmentChildren(children), { + groupHeading: GroupHeading, + }) + const childrenWithoutHeading = slots.groupHeading ? removeChildFromFragments(children, slots.groupHeading) : children // Default the group heading to one level below the NavList.Heading (h3 under an // h2, h4 under an h3), falling back to h3 when there is no NavList.Heading. To // use a different level, pass NavList.GroupHeading with an explicit `as` instead. @@ -376,12 +408,14 @@ const Group: React.FC = ({title, children, hideDivider, ...pr <> {!hideDivider && } - {title ? ( + {slots.groupHeading ? ( + React.cloneElement(slots.groupHeading, {headingWrapElement: 'div'}) + ) : title ? ( {title} ) : null} - {children} + {childrenWithoutHeading} ) @@ -495,7 +529,7 @@ 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 = ({as, className, ...rest}) => { +const GroupHeadingImpl: React.FC = ({as, className, ...rest}) => { const headingLevel = React.useContext(NavListHeadingLevelContext) const resolvedAs = as ?? (headingLevel ? levelToHeadingTag(headingLevel + 1) : 'h3') return ( @@ -509,6 +543,8 @@ const GroupHeading: React.FC = ({as, className, ...res ) } +const GroupHeading = asSlot(GroupHeadingImpl, ActionList.GroupHeading) + // ---------------------------------------------------------------------------- // Export