From 02f8be2e8b778361cb1a3f042c3f7e7e88692adc Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Fri, 18 Oct 2024 15:54:22 -0400 Subject: [PATCH 1/9] feat(LabelGroup): render as list by default --- packages/react/src/Label/Label.tsx | 17 ++- packages/react/src/LabelGroup/LabelGroup.tsx | 103 +++++++++------ .../src/LabelGroup/LabelGroupContext.tsx | 5 + .../react/src/__tests__/LabelGroup.test.tsx | 121 ++++++++++++++++++ 4 files changed, 201 insertions(+), 45 deletions(-) create mode 100644 packages/react/src/LabelGroup/LabelGroupContext.tsx diff --git a/packages/react/src/Label/Label.tsx b/packages/react/src/Label/Label.tsx index 7c2f6db5960..81b9f531af9 100644 --- a/packages/react/src/Label/Label.tsx +++ b/packages/react/src/Label/Label.tsx @@ -2,13 +2,14 @@ import {clsx} from 'clsx' import {useFeatureFlag} from '../FeatureFlags' import Box from '../Box' import classes from './Label.module.css' -import React from 'react' +import React, {useContext} from 'react' import styled from 'styled-components' import {variant} from 'styled-system' import {get} from '../constants' import type {BetterSystemStyleObject, SxProp} from '../sx' import sx from '../sx' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' +import {LabelGroupContext} from '../LabelGroup/LabelGroupContext' export type LabelProps = { /** The color of the label */ @@ -101,8 +102,9 @@ const StyledLabel = styled.span` const Label = React.forwardRef(function Label({as, size = 'small', variant = 'default', className, ...rest}, ref) { const enabled = useFeatureFlag('primer_react_css_modules_ga') + const {isList} = useContext(LabelGroupContext) if (enabled) { - const Component = as || 'span' + const Component = as || (isList ? 'li' : 'span') if (rest.sx) { return ( + ) }) as PolymorphicForwardRefComponent<'span', LabelProps> export default Label diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index e39d389ddd1..66cddf4ec9a 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -10,8 +10,11 @@ import {Button, IconButton} from '../Button' import {useTheme} from '../ThemeProvider' import type {SxProp} from '../sx' import sx from '../sx' +import {LabelGroupContext} from './LabelGroupContext' export type LabelGroupProps = { + /** Customize the element type of the rendered container */ + as?: React.ElementType /** How hidden tokens should be shown. `'inline'` shows the hidden tokens after the visible tokens. `'overlay'` shows all tokens in an overlay that appears on top of the visible tokens. */ overflowStyle?: 'inline' | 'overlay' /** How many tokens to show. `'auto'` truncates the tokens to fit in the parent container. Passing a number will truncate after that number tokens. If this is undefined, tokens will never be truncated. */ @@ -30,6 +33,12 @@ const StyledLabelGroupContainer = styled.div` flex-wrap: wrap; } + &[data-list] { + padding-inline-start: 0; + margin-block-start: 0; + margin-block-end: 0; + } + ${sx}; ` @@ -54,7 +63,7 @@ const ItemWrapper = styled.div` // Calculates the width of the overlay to cover the labels/tokens and the expand button. const getOverlayWidth = ( buttonClientRect: DOMRect, - containerRef: React.RefObject, + containerRef: React.RefObject, overlayPaddingPx: number, ) => overlayPaddingPx + buttonClientRect.right - (containerRef.current?.getBoundingClientRect().left || 0) @@ -148,8 +157,9 @@ const LabelGroup: React.FC> = ({ visibleChildCount, overflowStyle = 'overlay', sx: sxProp, + as = 'ul', }) => { - const containerRef = React.useRef(null) + const containerRef = React.useRef(null) const collapseButtonRef = React.useRef(null) const firstHiddenIndexRef = React.useRef(undefined) const [visibilityMap, setVisibilityMap] = React.useState>({}) @@ -317,51 +327,60 @@ const LabelGroup: React.FC> = ({ } }, [overflowStyle, isOverflowShown]) + const isList = as == 'ul' || as === 'ol' + // If truncation is enabled, we need to render based on truncation logic. - return visibleChildCount ? ( - - {React.Children.map(children, (child, index) => ( - + {visibleChildCount ? ( + - {child} - - ))} - {overflowStyle === 'inline' ? ( - + {React.Children.map(children, (child, index) => ( + + {child} + + ))} + {overflowStyle === 'inline' ? ( + + ) : ( + + {children} + + )} + ) : ( - + {children} - + )} - - ) : ( - - {children} - + ) } diff --git a/packages/react/src/LabelGroup/LabelGroupContext.tsx b/packages/react/src/LabelGroup/LabelGroupContext.tsx new file mode 100644 index 00000000000..fa236109724 --- /dev/null +++ b/packages/react/src/LabelGroup/LabelGroupContext.tsx @@ -0,0 +1,5 @@ +import {createContext} from 'react' + +export const LabelGroupContext = createContext<{ + isList?: boolean +}>({}) diff --git a/packages/react/src/__tests__/LabelGroup.test.tsx b/packages/react/src/__tests__/LabelGroup.test.tsx index b2e8f6192f5..2ff6a70f087 100644 --- a/packages/react/src/__tests__/LabelGroup.test.tsx +++ b/packages/react/src/__tests__/LabelGroup.test.tsx @@ -177,4 +177,125 @@ describe('LabelGroup', () => { expect(document.activeElement).toEqual(getByText('+2').closest('button')) }) + + describe('should render as ul by default', () => { + it('without truncation', () => { + const {getByRole} = HTMLRender( + + + + + + + + + , + ) + const list = getByRole('list') + expect(list).not.toBeNull() + expect(list.tagName).toBe('UL') + expect(list).toHaveAttribute('data-list', 'true') + expect(list.querySelectorAll('li')).toHaveLength(5) + }) + + it('with truncation', () => { + const {getByRole} = HTMLRender( + + + + + + + + + , + ) + const list = getByRole('list') + expect(list).not.toBeNull() + expect(list.tagName).toBe('UL') + expect(list).toHaveAttribute('data-list', 'true') + expect(list.querySelectorAll('li')).toHaveLength(5) + }) + }) + + describe('should render as custom element when `as` is provided', () => { + it('without truncation', () => { + const {queryByRole, container} = HTMLRender( + + + + + + + + + , + ) + const list = queryByRole('list') + expect(list).toBeNull() + const labelGroupDiv = container.querySelectorAll('div')[1] + expect(labelGroupDiv.querySelectorAll('li')).toHaveLength(0) + expect(labelGroupDiv.querySelectorAll('span')).toHaveLength(5) + expect(labelGroupDiv).not.toHaveAttribute('data-list') + }) + + it('with truncation', () => { + const {queryByRole, container} = HTMLRender( + + + + + + + + + , + ) + const list = queryByRole('list') + expect(list).toBeNull() + const labelGroupDiv = container.querySelectorAll('div')[1] + expect(labelGroupDiv.querySelectorAll('li')).toHaveLength(0) + expect(labelGroupDiv.querySelectorAll(':scope > span')).toHaveLength(5) + expect(labelGroupDiv).not.toHaveAttribute('data-list') + }) + }) + + describe('should render children as list items when rendered as ol', () => { + it('without truncation', () => { + const {getByRole} = HTMLRender( + + + + + + + + + , + ) + const list = getByRole('list') + expect(list).not.toBeNull() + expect(list.tagName).toBe('OL') + expect(list).toHaveAttribute('data-list', 'true') + expect(list.querySelectorAll('li')).toHaveLength(5) + }) + it('with truncation', () => { + const {getByRole} = HTMLRender( + + + + + + + + + , + ) + const list = getByRole('list') + expect(list).not.toBeNull() + expect(list.tagName).toBe('OL') + expect(list).toHaveAttribute('data-list', 'true') + expect(list.querySelectorAll('li')).toHaveLength(5) + }) + }) }) From ccb002fd67a8e3052be885407abcdeddb6e04342 Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Fri, 18 Oct 2024 16:15:29 -0400 Subject: [PATCH 2/9] docs(LabelGroup): add as props to docs.json --- packages/react/src/LabelGroup/LabelGroup.docs.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/react/src/LabelGroup/LabelGroup.docs.json b/packages/react/src/LabelGroup/LabelGroup.docs.json index 34fafdde94f..51e64eef176 100644 --- a/packages/react/src/LabelGroup/LabelGroup.docs.json +++ b/packages/react/src/LabelGroup/LabelGroup.docs.json @@ -17,6 +17,12 @@ "description": "How many tokens to show. `'auto'` truncates the tokens to fit in the parent container. Passing a number will truncate after that number tokens. If this is undefined, tokens will never be truncated.", "defaultValue": "", "type": "'auto' | number" + }, + { + "name": "as", + "description": "Customize the element type of the rendered container.", + "defaultValue": "ul", + "type": "React.ElementType" } ], "subcomponents": [] From 029b1f3d1a37207c3f0f3aab43a44f637fa63c34 Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Fri, 18 Oct 2024 16:46:08 -0400 Subject: [PATCH 3/9] fix(LabelGroup): lint --- packages/react/src/LabelGroup/LabelGroup.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index 66cddf4ec9a..ac9c44589c0 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -327,7 +327,7 @@ const LabelGroup: React.FC> = ({ } }, [overflowStyle, isOverflowShown]) - const isList = as == 'ul' || as === 'ol' + const isList = as === 'ul' || as === 'ol' // If truncation is enabled, we need to render based on truncation logic. return ( From a9922243c6fe0ace146bac925f07ba38a0cc0e62 Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Fri, 18 Oct 2024 17:16:35 -0400 Subject: [PATCH 4/9] fix(LabelGroup): action button inside li --- packages/react/src/LabelGroup/LabelGroup.tsx | 51 +++++++++++--------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index ac9c44589c0..99ea4dc99d5 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -37,6 +37,7 @@ const StyledLabelGroupContainer = styled.div` padding-inline-start: 0; margin-block-start: 0; margin-block-end: 0; + list-style-type: none; } ${sx}; @@ -350,30 +351,32 @@ const LabelGroup: React.FC> = ({ {child} ))} - {overflowStyle === 'inline' ? ( - - ) : ( - - {children} - - )} +
  • + {overflowStyle === 'inline' ? ( + + ) : ( + + {children} + + )} +
    • ) : ( From 0ffce7ff242b9502440f5af4289b69f854fe65e4 Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Mon, 21 Oct 2024 14:58:34 -0400 Subject: [PATCH 5/9] fix(LabelGroup): fix broken tests, small refactor --- packages/react/src/LabelGroup/LabelGroup.stories.tsx | 4 ++++ packages/react/src/LabelGroup/LabelGroup.tsx | 5 +++-- packages/react/src/__tests__/LabelGroup.test.tsx | 6 ++++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/react/src/LabelGroup/LabelGroup.stories.tsx b/packages/react/src/LabelGroup/LabelGroup.stories.tsx index 639446335d2..d123757c6ed 100644 --- a/packages/react/src/LabelGroup/LabelGroup.stories.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.stories.tsx @@ -69,6 +69,10 @@ export const Playground: StoryFn = ({ ) } +Playground.args = { + as: 'ul', +} + Playground.argTypes = { overflowStyle: { control: { diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index 99ea4dc99d5..1962b8360d9 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -329,6 +329,7 @@ const LabelGroup: React.FC> = ({ }, [overflowStyle, isOverflowShown]) const isList = as === 'ul' || as === 'ol' + const ToggleWrapper = isList ? 'li' : React.Fragment // If truncation is enabled, we need to render based on truncation logic. return ( @@ -351,7 +352,7 @@ const LabelGroup: React.FC> = ({ {child} ))} -
  • + {overflowStyle === 'inline' ? ( > = ({ {children} )} -
    • +     ) : ( diff --git a/packages/react/src/__tests__/LabelGroup.test.tsx b/packages/react/src/__tests__/LabelGroup.test.tsx index 2ff6a70f087..77a89f65581 100644 --- a/packages/react/src/__tests__/LabelGroup.test.tsx +++ b/packages/react/src/__tests__/LabelGroup.test.tsx @@ -214,7 +214,8 @@ describe('LabelGroup', () => { expect(list).not.toBeNull() expect(list.tagName).toBe('UL') expect(list).toHaveAttribute('data-list', 'true') - expect(list.querySelectorAll('li')).toHaveLength(5) + // account for "show more" button + expect(list.querySelectorAll('li')).toHaveLength(6) }) }) @@ -295,7 +296,8 @@ describe('LabelGroup', () => { expect(list).not.toBeNull() expect(list.tagName).toBe('OL') expect(list).toHaveAttribute('data-list', 'true') - expect(list.querySelectorAll('li')).toHaveLength(5) + // account for "show more" button + expect(list.querySelectorAll('li')).toHaveLength(6) }) }) }) From 9de15849d724b86c2690c7a8cc5aa49225d6e94f Mon Sep 17 00:00:00 2001 From: Marie Lucca <40550942+francinelucca@users.noreply.github.com> Date: Mon, 21 Oct 2024 14:59:24 -0400 Subject: [PATCH 6/9] Create blue-eggs-suffer.md --- .changeset/blue-eggs-suffer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/blue-eggs-suffer.md diff --git a/.changeset/blue-eggs-suffer.md b/.changeset/blue-eggs-suffer.md new file mode 100644 index 00000000000..7f2aba3a444 --- /dev/null +++ b/.changeset/blue-eggs-suffer.md @@ -0,0 +1,5 @@ +--- +"@primer/react": minor +--- + +feat(LabelGroup): render as list by default From 34bc9673a8197772a2c57e22deabbf026acef89c Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Wed, 23 Oct 2024 17:38:44 -0400 Subject: [PATCH 7/9] fix(LabelGroup): wrap children in li instead of using context --- packages/react/src/Label/Label.tsx | 17 +-- packages/react/src/LabelGroup/LabelGroup.tsx | 105 +++++++++--------- .../src/LabelGroup/LabelGroupContext.tsx | 5 - 3 files changed, 55 insertions(+), 72 deletions(-) delete mode 100644 packages/react/src/LabelGroup/LabelGroupContext.tsx diff --git a/packages/react/src/Label/Label.tsx b/packages/react/src/Label/Label.tsx index 81b9f531af9..79e65ebe198 100644 --- a/packages/react/src/Label/Label.tsx +++ b/packages/react/src/Label/Label.tsx @@ -2,14 +2,13 @@ import {clsx} from 'clsx' import {useFeatureFlag} from '../FeatureFlags' import Box from '../Box' import classes from './Label.module.css' -import React, {useContext} from 'react' +import React from 'react' import styled from 'styled-components' import {variant} from 'styled-system' import {get} from '../constants' import type {BetterSystemStyleObject, SxProp} from '../sx' import sx from '../sx' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' -import {LabelGroupContext} from '../LabelGroup/LabelGroupContext' export type LabelProps = { /** The color of the label */ @@ -102,9 +101,8 @@ const StyledLabel = styled.span` const Label = React.forwardRef(function Label({as, size = 'small', variant = 'default', className, ...rest}, ref) { const enabled = useFeatureFlag('primer_react_css_modules_ga') - const {isList} = useContext(LabelGroupContext) if (enabled) { - const Component = as || (isList ? 'li' : 'span') + const Component = as || 'span' if (rest.sx) { return ( - ) + return }) as PolymorphicForwardRefComponent<'span', LabelProps> export default Label diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index 1962b8360d9..6fcb2738850 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -10,7 +10,6 @@ import {Button, IconButton} from '../Button' import {useTheme} from '../ThemeProvider' import type {SxProp} from '../sx' import sx from '../sx' -import {LabelGroupContext} from './LabelGroupContext' export type LabelGroupProps = { /** Customize the element type of the rendered container */ @@ -332,59 +331,59 @@ const LabelGroup: React.FC> = ({ const ToggleWrapper = isList ? 'li' : React.Fragment // If truncation is enabled, we need to render based on truncation logic. - return ( - - {visibleChildCount ? ( - + {React.Children.map(children, (child, index) => ( + - {React.Children.map(children, (child, index) => ( - - {child} - - ))} - - {overflowStyle === 'inline' ? ( - - ) : ( - - {children} - - )} - - - ) : ( - - {children} - - )} - + {child} + + ))} + + {overflowStyle === 'inline' ? ( + + ) : ( + + {children} + + )} + + + ) : ( + + {isList + ? React.Children.map(children, child => { + return
  • {child}
    • + }) + : children} +     ) } diff --git a/packages/react/src/LabelGroup/LabelGroupContext.tsx b/packages/react/src/LabelGroup/LabelGroupContext.tsx deleted file mode 100644 index fa236109724..00000000000 --- a/packages/react/src/LabelGroup/LabelGroupContext.tsx +++ /dev/null @@ -1,5 +0,0 @@ -import {createContext} from 'react' - -export const LabelGroupContext = createContext<{ - isList?: boolean -}>({}) From 4d74146d6b8042a4f04201245f43210a537a0b66 Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Wed, 23 Oct 2024 17:40:47 -0400 Subject: [PATCH 8/9] fix(Label): revert changes --- packages/react/src/Label/Label.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/Label/Label.tsx b/packages/react/src/Label/Label.tsx index 79e65ebe198..7c2f6db5960 100644 --- a/packages/react/src/Label/Label.tsx +++ b/packages/react/src/Label/Label.tsx @@ -117,7 +117,7 @@ const Label = React.forwardRef(function Label({as, size = 'small', variant = 'de } return } - return + return }) as PolymorphicForwardRefComponent<'span', LabelProps> export default Label From c9675efcbeb0189df5f7518d9383c5726f6f88f6 Mon Sep 17 00:00:00 2001 From: Marie Lucca Date: Wed, 30 Oct 2024 22:43:11 -0400 Subject: [PATCH 9/9] fix(LabelGroup): use add key to children map --- packages/react/src/LabelGroup/LabelGroup.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index 6fcb2738850..5aaf6f8edd8 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -345,6 +345,7 @@ const LabelGroup: React.FC> = ({ data-index={index} className={hiddenItemIds.includes(index.toString()) ? 'ItemWrapper--hidden' : undefined} as={isList ? 'li' : 'span'} + key={index} > {child} @@ -379,8 +380,8 @@ const LabelGroup: React.FC> = ({ ) : ( {isList - ? React.Children.map(children, child => { - return
  • {child}
    • + ? React.Children.map(children, (child, index) => { + return
  • {child}
    • }) : children}