ActionList Fixes (#1215)

Co-authored-by: Clay Miller <clay@smockle.com>
primer · May 12, 2021 · c63fa4b · c63fa4b · vercel · May 12, 2021
1 parent 4ab3d17
commit c63fa4b
Show file tree

Hide file tree

Showing 10 changed files with 175 additions and 29 deletions.
diff --git a/.changeset/blue-items-repeat.md b/.changeset/blue-items-repeat.md
@@ -0,0 +1,5 @@
+---
+"@primer/components": patch
+---
+
+Hide divider before `ActionList.Group`s with filled header
diff --git a/.changeset/hip-bugs-rule.md b/.changeset/hip-bugs-rule.md
@@ -0,0 +1,5 @@
+---
+"@primer/components": patch
+---
+
+Align `Item` description to when rendered in-line
diff --git a/.changeset/long-eagles-fold.md b/.changeset/long-eagles-fold.md
@@ -0,0 +1,5 @@
+---
+"@primer/components": patch
+---
+
+Allow `focusZoneSettings` to be passed into `AnchoredOverlay`
diff --git a/.changeset/lovely-poems-crash.md b/.changeset/lovely-poems-crash.md
@@ -0,0 +1,5 @@
+---
+"@primer/components": patch
+---
+
+Add `selectionVariant: 'multiple'` for `Item`s.  These will use a checkbox input instead of a checkmark icon for selected state
diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx
@@ -55,6 +55,11 @@ export interface ItemProps extends Omit<React.ComponentPropsWithoutRef<'div'>, '
    */
   selected?: boolean
 
+  /**
+   *  For `Item`s which can be selected, whether `multiple` `Item`s or a `single` `Item` can be selected
+   */
+  selectionVariant?: 'single' | 'multiple'
+
   /**
    * Designates the group that an item belongs to.
    */
@@ -129,6 +134,7 @@ const StyledItem = styled.div<{variant: ItemProps['variant']; item?: ItemInput}
 `
 
 const StyledTextContainer = styled.div<{descriptionVariant: ItemProps['descriptionVariant']}>`
+  display: flex;
   flex-direction: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 'row' : 'column')};
 `
 
@@ -163,8 +169,9 @@ const TrailingVisualContainer = styled(BaseVisualContainer)`
   justify-content: flex-end;
 `
 
-const DescriptionContainer = styled.span`
+const DescriptionContainer = styled.span<{descriptionVariant: ItemProps['descriptionVariant']}>`
   color: ${get('colors.text.secondary')};
+  margin-left: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? get('space.2') : 0)};
 `
 
 /**
@@ -176,6 +183,7 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
     description,
     descriptionVariant = 'inline',
     selected,
+    selectionVariant,
     leadingVisual: LeadingVisual,
     trailingIcon: TrailingIcon,
     trailingText,
@@ -195,6 +203,12 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
         return
       }
       onKeyPress?.(event)
+      const isCheckbox = event.target instanceof HTMLInputElement && event.target.type === 'checkbox'
+      if (isCheckbox && event.key === ' ') {
+        // space key on a checkbox will also trigger a click event.  Ignore the space key so we don't get double events
+        return
+      }
+
       if (!event.defaultPrevented && [' ', 'Enter'].includes(event.key)) {
         onAction?.(itemProps as ItemProps, event)
       }
@@ -225,7 +239,21 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
       onKeyPress={keyPressHandler}
       onClick={clickHandler}
     >
-      {!!selected === selected && <LeadingVisualContainer>{selected && <CheckIcon />}</LeadingVisualContainer>}
+      {!!selected === selected && (
+        <LeadingVisualContainer>
+          {selectionVariant === 'multiple' ? (
+            <>
+              {/*
+               * readOnly is required because we are doing a one-way bind to `checked`.
+               * aria-readonly="false" tells screen that they can still interact with the checkbox
+               */}
+              <input type="checkbox" checked={selected} aria-label={text} readOnly aria-readonly="false" />
+            </>
+          ) : (
+            selected && <CheckIcon />
+          )}
+        </LeadingVisualContainer>
+      )}
       {LeadingVisual && (
         <LeadingVisualContainer variant={variant} disabled={disabled}>
           <LeadingVisual />
@@ -235,7 +263,9 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
       {(text || description) && (
         <StyledTextContainer descriptionVariant={descriptionVariant}>
           {text && <div>{text}</div>}
-          {description && <DescriptionContainer>{description}</DescriptionContainer>}
+          {description && (
+            <DescriptionContainer descriptionVariant={descriptionVariant}>{description}</DescriptionContainer>
+          )}
         </StyledTextContainer>
       )}
       {(TrailingIcon || trailingText) && (

diff --git a/src/ActionList/List.tsx b/src/ActionList/List.tsx
@@ -45,6 +45,11 @@ export interface ListPropsBase {
    * - `"full"` - `List` children are flush (vertically and horizontally) with `List` edges
    */
   variant?: 'inset' | 'full'
+
+  /**
+   *  For `Item`s which can be selected, whether `multiple` `Item`s or a `single` `Item` can be selected
+   */
+  selectionVariant?: 'single' | 'multiple'
 }
 
 /**
@@ -82,6 +87,12 @@ export type ListProps = ListPropsBase | GroupedListProps
 
 const StyledList = styled.div`
   font-size: ${get('fontSizes.1')};
+  /* 14px font-size * 1.428571429 = 20px line height
+   *
+   * TODO: When rem-based spacing on a 4px scale lands, replace
+   * hardcoded '20px'
+   */
+  line-height: 20px;
 `
 
 /**
@@ -138,7 +149,15 @@ export function List(props: ListProps): JSX.Element {
   const renderItem = (itemProps: ItemInput, item: ItemInput) => {
     const ItemComponent = ('renderItem' in itemProps && itemProps.renderItem) || props.renderItem || Item
     const key = itemProps.key ?? itemProps.id?.toString() ?? uniqueId()
-    return <ItemComponent {...itemProps} key={key} sx={{...itemStyle, ...itemProps.sx}} item={item} />
+    return (
+      <ItemComponent
+        selectionVariant={props.selectionVariant}
+        {...itemProps}
+        key={key}
+        sx={{...itemStyle, ...itemProps.sx}}
+        item={item}
+      />
+    )
   }
 
   /**
@@ -188,24 +207,29 @@ export function List(props: ListProps): JSX.Element {
 
   return (
     <StyledList {...props}>
-      {groups?.map(({header, ...groupProps}, index) => (
-        <React.Fragment key={groupProps.groupId}>
-          {renderGroup({
-            sx: {
-              ...(index === 0 && firstGroupStyle),
-              ...(index === groups.length - 1 && lastGroupStyle)
-            },
-            ...(header && {
-              header: {
-                ...header,
-                sx: {...headerStyle, ...header?.sx}
-              }
-            }),
-            ...groupProps
-          })}
-          {index + 1 !== groups.length && <Divider key={`${groupProps.groupId}-divider`} />}
-        </React.Fragment>
-      ))}
+      {groups?.map(({header, ...groupProps}, index) => {
+        const hasFilledHeader = header?.variant === 'filled'
+        const shouldShowDivider = index > 0 && !hasFilledHeader
+        return (
+          <React.Fragment key={groupProps.groupId}>
+            {shouldShowDivider ? <Divider key={`${groupProps.groupId}-divider`} /> : null}
+            {renderGroup({
+              sx: {
+                ...(index === 0 && firstGroupStyle),
+                ...(index === groups.length - 1 && lastGroupStyle),
+                ...(index > 0 && !shouldShowDivider && {mt: 2})
+              },
+              ...(header && {
+                header: {
+                  ...header,
+                  sx: {...headerStyle, ...header?.sx}
+                }
+              }),
+              ...groupProps
+            })}
+          </React.Fragment>
+        )
+      })}
     </StyledList>
   )
 }
diff --git a/src/AnchoredOverlay/AnchoredOverlay.tsx b/src/AnchoredOverlay/AnchoredOverlay.tsx
@@ -4,6 +4,7 @@ import {useFocusTrap} from '../hooks/useFocusTrap'
 import {useFocusZone} from '../hooks/useFocusZone'
 import {useAnchoredPosition, useRenderForcingRef} from '../hooks'
 import {uniqueId} from '../utils/uniqueId'
+import {FocusZoneSettings} from '../behaviors/focusZone'
 
 export interface AnchoredOverlayProps extends Pick<OverlayProps, 'height' | 'width'> {
   /**
@@ -31,6 +32,11 @@ export interface AnchoredOverlayProps extends Pick<OverlayProps, 'height' | 'wid
    * Props to be spread on the internal `Overlay` component.
    */
   overlayProps?: Partial<OverlayProps>
+
+  /**
+   * Settings to apply to the Focus Zone on the internal `Overlay` component.
+   */
+  focusZoneSettings?: Partial<FocusZoneSettings>
 }
 
 /**
@@ -44,8 +50,9 @@ export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({
   onOpen,
   onClose,
   height,
+  width,
   overlayProps,
-  width
+  focusZoneSettings
 }) => {
   const anchorRef = useRef<HTMLElement>(null)
   const [overlayRef, updateOverlayRef] = useRenderForcingRef<HTMLDivElement>()
@@ -85,7 +92,11 @@ export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({
     return position && {top: `${position.top}px`, left: `${position.left}px`}
   }, [position])
 
-  useFocusZone({containerRef: overlayRef, disabled: !open || !position})
+  useFocusZone({
+    containerRef: overlayRef,
+    disabled: !open || !position,
+    ...focusZoneSettings
+  })
   useFocusTrap({containerRef: overlayRef, disabled: !open || !position})
 
   return (

diff --git a/src/Overlay.tsx b/src/Overlay.tsx
@@ -37,7 +37,6 @@ const StyledOverlay = styled.div<StyledOverlayProps & SystemCommonProps & System
   height: ${props => heightMap[props.height || 'auto']};
   width: ${props => widthMap[props.width || 'auto']};
   border-radius: 12px;
-  overflow: hidden;
   animation: overlay-appear 200ms ${get('animation.easeOutCubic')};
 
   @keyframes overlay-appear {

diff --git a/src/__tests__/__snapshots__/ActionList.tsx.snap b/src/__tests__/__snapshots__/ActionList.tsx.snap
@@ -8,6 +8,7 @@ exports[`ActionList renders consistently 1`] = `
 
 .c0 {
   font-size: 14px;
+  line-height: 20px;
 }
 
 <div

diff --git a/src/stories/ActionList.stories.tsx b/src/stories/ActionList.stories.tsx
@@ -47,7 +47,9 @@ export default meta
 const ErsatzOverlay = styled.div`
   border-radius: 12px;
   box-shadow: 0 1px 3px rgba(0, 0, 0, 0.12), 0 8px 24px rgba(149, 157, 165, 0.2);
-  overflow: hidden;
+  position: absolute;
+  min-width: 192px;
+  max-width: 640px;
 `
 
 export function ActionsStory(): JSX.Element {
@@ -98,7 +100,49 @@ export function SimpleListStory(): JSX.Element {
 }
 SimpleListStory.storyName = 'Simple List'
 
-export function ComplexListStory(): JSX.Element {
+const selectListItems = new Array(6).fill(undefined).map((_, i) => {
+  return {
+    text: `Item ${i}`,
+    id: i
+  }
+})
+
+export function SingleSelectListStory(): JSX.Element {
+  return (
+    <>
+      <h1>Single Select List</h1>
+      <ErsatzOverlay>
+        <ActionList
+          items={selectListItems.map((item, index) => ({
+            ...item,
+            selected: index === 1
+          }))}
+        />
+      </ErsatzOverlay>
+    </>
+  )
+}
+SingleSelectListStory.storyName = 'Single Select'
+
+export function MultiSelectListStory(): JSX.Element {
+  return (
+    <>
+      <h1>Multi Select List</h1>
+      <ErsatzOverlay>
+        <ActionList
+          selectionVariant="multiple"
+          items={selectListItems.map((item, index) => ({
+            ...item,
+            selected: index === 1 || index === 3
+          }))}
+        />
+      </ErsatzOverlay>
+    </>
+  )
+}
+MultiSelectListStory.storyName = 'Multi Select'
+
+export function ComplexListInsetVariantStory(): JSX.Element {
   const StyledDiv = styled.div`
     ${sx}
   `
@@ -132,7 +176,13 @@ export function ComplexListStory(): JSX.Element {
           ]}
           items={[
             {leadingVisual: TypographyIcon, text: 'Rename', groupId: '0'},
-            {leadingVisual: VersionsIcon, text: 'Duplicate', groupId: '0'},
+            {
+              leadingVisual: VersionsIcon,
+              text: 'Duplicate',
+              description: 'Create a copy',
+              descriptionVariant: 'inline',
+              groupId: '0'
+            },
             {
               leadingVisual: SearchIcon,
               text: 'repo:github/memex,github/github',
@@ -163,7 +213,18 @@ export function ComplexListStory(): JSX.Element {
           ]}
         />
       </ErsatzOverlay>
+    </>
+  )
+}
+ComplexListInsetVariantStory.storyName = 'Complex List — Inset Variant'
 
+export function ComplexListFullVariantStory(): JSX.Element {
+  const StyledDiv = styled.div`
+    ${sx}
+  `
+  return (
+    <>
+      <h1>Complex List</h1>
       <h2>Full Variant</h2>
       <ErsatzOverlay>
         <ActionList
@@ -226,7 +287,7 @@ export function ComplexListStory(): JSX.Element {
     </>
   )
 }
-ComplexListStory.storyName = 'Complex List'
+ComplexListFullVariantStory.storyName = 'Complex List — Full Variant'
 
 export function HeaderStory(): JSX.Element {
   return (