feat(LabelGroup): render as list by default (#5156)

* feat(LabelGroup): render as list by default * docs(LabelGroup): add as props to docs.json * fix(LabelGroup): lint * fix(LabelGroup): action button inside li * fix(LabelGroup): fix broken tests, small refactor * Create blue-eggs-suffer.md * fix(LabelGroup): wrap children in li instead of using context * fix(Label): revert changes * fix(LabelGroup): use add key to children map
primer · Oct 31, 2024 · 8e58e4d · 8e58e4d
1 parent 7efa934
commit 8e58e4d
Show file tree

Hide file tree

Showing 5 changed files with 189 additions and 28 deletions.
diff --git a/.changeset/blue-eggs-suffer.md b/.changeset/blue-eggs-suffer.md
@@ -0,0 +1,5 @@
+---
+"@primer/react": minor
+---
+
+feat(LabelGroup): render as list by default
diff --git 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": []

diff --git a/packages/react/src/LabelGroup/LabelGroup.stories.tsx b/packages/react/src/LabelGroup/LabelGroup.stories.tsx
@@ -69,6 +69,10 @@ export const Playground: StoryFn = ({
     </ResizableContainer>
   )
 }
+Playground.args = {
+  as: 'ul',
+}
+
 Playground.argTypes = {
   overflowStyle: {
     control: {

diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx
@@ -12,6 +12,8 @@ import type {SxProp} from '../sx'
 import sx from '../sx'
 
 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 +32,13 @@ const StyledLabelGroupContainer = styled.div<SxProp>`
     flex-wrap: wrap;
   }
 
+  &[data-list] {
+    padding-inline-start: 0;
+    margin-block-start: 0;
+    margin-block-end: 0;
+    list-style-type: none;
+  }
+
   ${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<HTMLDivElement>,
+  containerRef: React.RefObject<HTMLElement>,
   overlayPaddingPx: number,
 ) => overlayPaddingPx + buttonClientRect.right - (containerRef.current?.getBoundingClientRect().left || 0)
 
@@ -148,8 +157,9 @@ const LabelGroup: React.FC<React.PropsWithChildren<LabelGroupProps>> = ({
   visibleChildCount,
   overflowStyle = 'overlay',
   sx: sxProp,
+  as = 'ul',
 }) => {
-  const containerRef = React.useRef<HTMLDivElement>(null)
+  const containerRef = React.useRef<HTMLElement>(null)
   const collapseButtonRef = React.useRef<HTMLButtonElement>(null)
   const firstHiddenIndexRef = React.useRef<number | undefined>(undefined)
   const [visibilityMap, setVisibilityMap] = React.useState<Record<string, boolean>>({})
@@ -317,50 +327,63 @@ const LabelGroup: React.FC<React.PropsWithChildren<LabelGroupProps>> = ({
     }
   }, [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 visibleChildCount ? (
     <StyledLabelGroupContainer
       ref={containerRef}
       data-overflow={overflowStyle === 'inline' && isOverflowShown ? 'inline' : undefined}
+      data-list={isList || undefined}
       sx={sxProp}
+      as={as}
     >
       {React.Children.map(children, (child, index) => (
         <ItemWrapper
           // data-index is used as an identifier we can use in the IntersectionObserver
           data-index={index}
           className={hiddenItemIds.includes(index.toString()) ? 'ItemWrapper--hidden' : undefined}
+          as={isList ? 'li' : 'span'}
+          key={index}
         >
           {child}
         </ItemWrapper>
       ))}
-      {overflowStyle === 'inline' ? (
-        <InlineToggle
-          collapseButtonRef={collapseButtonRef}
-          collapseInlineExpandedChildren={collapseInlineExpandedChildren}
-          expandButtonRef={expandButtonRef}
-          hiddenItemIds={hiddenItemIds}
-          isOverflowShown={isOverflowShown}
-          showAllTokensInline={showAllTokensInline}
-          totalLength={React.Children.toArray(children).length}
-        />
-      ) : (
-        <OverlayToggle
-          closeOverflowOverlay={closeOverflowOverlay}
-          expandButtonRef={expandButtonRef}
-          hiddenItemIds={hiddenItemIds}
-          isOverflowShown={isOverflowShown}
-          openOverflowOverlay={openOverflowOverlay}
-          overlayPaddingPx={overlayPaddingPx}
-          overlayWidth={overlayWidth}
-          totalLength={React.Children.toArray(children).length}
-        >
-          {children}
-        </OverlayToggle>
-      )}
+      <ToggleWrapper>
+        {overflowStyle === 'inline' ? (
+          <InlineToggle
+            collapseButtonRef={collapseButtonRef}
+            collapseInlineExpandedChildren={collapseInlineExpandedChildren}
+            expandButtonRef={expandButtonRef}
+            hiddenItemIds={hiddenItemIds}
+            isOverflowShown={isOverflowShown}
+            showAllTokensInline={showAllTokensInline}
+            totalLength={React.Children.toArray(children).length}
+          />
+        ) : (
+          <OverlayToggle
+            closeOverflowOverlay={closeOverflowOverlay}
+            expandButtonRef={expandButtonRef}
+            hiddenItemIds={hiddenItemIds}
+            isOverflowShown={isOverflowShown}
+            openOverflowOverlay={openOverflowOverlay}
+            overlayPaddingPx={overlayPaddingPx}
+            overlayWidth={overlayWidth}
+            totalLength={React.Children.toArray(children).length}
+          >
+            {children}
+          </OverlayToggle>
+        )}
+      </ToggleWrapper>
     </StyledLabelGroupContainer>
   ) : (
-    <StyledLabelGroupContainer data-overflow="inline" sx={sxProp}>
-      {children}
+    <StyledLabelGroupContainer data-overflow="inline" data-list={isList || undefined} sx={sxProp} as={as}>
+      {isList
+        ? React.Children.map(children, (child, index) => {
+            return <li key={index}>{child}</li>
+          })
+        : children}
     </StyledLabelGroupContainer>
   )
 }

diff --git a/packages/react/src/__tests__/LabelGroup.test.tsx b/packages/react/src/__tests__/LabelGroup.test.tsx
@@ -177,4 +177,127 @@ describe('LabelGroup', () => {
 
     expect(document.activeElement).toEqual(getByText('+2').closest('button'))
   })
+
+  describe('should render as ul by default', () => {
+    it('without truncation', () => {
+      const {getByRole} = HTMLRender(
+        <ThemeAndStyleContainer>
+          <LabelGroup>
+            <Label>One</Label>
+            <Label>Two</Label>
+            <Label>Three</Label>
+            <Label>Four</Label>
+            <Label>Five</Label>
+          </LabelGroup>
+        </ThemeAndStyleContainer>,
+      )
+      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(
+        <ThemeAndStyleContainer>
+          <LabelGroup visibleChildCount={3}>
+            <Label>One</Label>
+            <Label>Two</Label>
+            <Label>Three</Label>
+            <Label>Four</Label>
+            <Label>Five</Label>
+          </LabelGroup>
+        </ThemeAndStyleContainer>,
+      )
+      const list = getByRole('list')
+      expect(list).not.toBeNull()
+      expect(list.tagName).toBe('UL')
+      expect(list).toHaveAttribute('data-list', 'true')
+      // account for "show more" button
+      expect(list.querySelectorAll('li')).toHaveLength(6)
+    })
+  })
+
+  describe('should render as custom element when `as` is provided', () => {
+    it('without truncation', () => {
+      const {queryByRole, container} = HTMLRender(
+        <ThemeAndStyleContainer>
+          <LabelGroup as="div">
+            <Label>One</Label>
+            <Label>Two</Label>
+            <Label>Three</Label>
+            <Label>Four</Label>
+            <Label>Five</Label>
+          </LabelGroup>
+        </ThemeAndStyleContainer>,
+      )
+      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(
+        <ThemeAndStyleContainer>
+          <LabelGroup as="div" visibleChildCount={2}>
+            <Label>One</Label>
+            <Label>Two</Label>
+            <Label>Three</Label>
+            <Label>Four</Label>
+            <Label>Five</Label>
+          </LabelGroup>
+        </ThemeAndStyleContainer>,
+      )
+      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(
+        <ThemeAndStyleContainer>
+          <LabelGroup as={'ol'}>
+            <Label>One</Label>
+            <Label>Two</Label>
+            <Label>Three</Label>
+            <Label>Four</Label>
+            <Label>Five</Label>
+          </LabelGroup>
+        </ThemeAndStyleContainer>,
+      )
+      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(
+        <ThemeAndStyleContainer>
+          <LabelGroup as={'ol'} visibleChildCount={1}>
+            <Label>One</Label>
+            <Label>Two</Label>
+            <Label>Three</Label>
+            <Label>Four</Label>
+            <Label>Five</Label>
+          </LabelGroup>
+        </ThemeAndStyleContainer>,
+      )
+      const list = getByRole('list')
+      expect(list).not.toBeNull()
+      expect(list.tagName).toBe('OL')
+      expect(list).toHaveAttribute('data-list', 'true')
+      // account for "show more" button
+      expect(list.querySelectorAll('li')).toHaveLength(6)
+    })
+  })
 })