Merge branch 'main' into mp/select-focus-outline-fix

mperrotti · web-flow · commit b44a517be819 · 2022-07-22T14:55:42.000-04:00
diff --git a/.changeset/chilly-frogs-act.md b/.changeset/chilly-frogs-act.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Support issues closed as not planned, and correct the standard issue closed backgroud colour
diff --git a/.changeset/cool-wolves-float.md b/.changeset/cool-wolves-float.md
diff --git a/.changeset/sweet-elephants-end.md b/.changeset/sweet-elephants-end.md
diff --git a/.changeset/tough-coats-allow.md b/.changeset/tough-coats-allow.md
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,19 @@
 # @primer/components
 
+## 35.5.0
+
+### Minor Changes
+
+- [#2180](https://github.com/primer/react/pull/2180) [`300025d1`](https://github.com/primer/react/commit/300025d11a010eb076f2327897744d06539bb355) Thanks [@mattcosta7](https://github.com/mattcosta7)! - Update the primer/primitives dependency
+
+### Patch Changes
+
+- [#2150](https://github.com/primer/react/pull/2150) [`63a2de51`](https://github.com/primer/react/commit/63a2de51c0d7de0d2640cc80435b1cead071f1f6) Thanks [@dgreif](https://github.com/dgreif)! - Ensure all files in `lib-esm` are in fact esm and not CommonJS
+
+* [#2145](https://github.com/primer/react/pull/2145) [`a2950ac4`](https://github.com/primer/react/commit/a2950ac40b9caeeb2b3d9883508d2d6a84980252) Thanks [@mperrotti](https://github.com/mperrotti)! - Updates SegmentedControl component's keyboard navigation to align with the recommendations of GitHub's accessibility team.
+
+- [#2143](https://github.com/primer/react/pull/2143) [`d9b161a0`](https://github.com/primer/react/commit/d9b161a0bde13a20a5972159a3f71e7be7475d7a) Thanks [@mperrotti](https://github.com/mperrotti)! - Fixes bugs in form components discovered while fixing/improving Storybook and docs.
+
 ## 35.4.0
 
 ### Minor Changes
diff --git a/docs/content/StateLabel.md b/docs/content/StateLabel.md
@@ -12,6 +12,7 @@ Use StateLabel components to show the status of an issue or pull request.
 <>
   <StateLabel status="issueOpened">Open</StateLabel>
   <StateLabel status="issueClosed">Closed</StateLabel>
+  <StateLabel status="issueClosedNotPlanned">Closed</StateLabel>
   <StateLabel status="pullOpened">Open</StateLabel>
   <StateLabel status="pullClosed">Closed</StateLabel>
   <StateLabel status="pullMerged">Merged</StateLabel>
@@ -22,8 +23,8 @@ Use StateLabel components to show the status of an issue or pull request.
 
 ## Component props
 
-| Name    | Type              | Default  | Description                                                                                                    |
-| :------ | :---------------- | :------: | :------------------------------------------------------------------------------------------------------------- |
-| variant | String            | 'normal' | a value of `small` or `normal` results in a smaller or larger version of the StateLabel.                       |
-| status  | String            |          | Can be one of `issueOpened`, `issueClosed`, `pullOpened`, `pullClosed`, `pullMerged`, `draft` or `issueDraft`. |
-| sx      | SystemStyleObject |    {}    | Style to be applied to the component                                                                           |
+| Name    | Type              | Default  | Description                                                                                                                             |
+| :------ | :---------------- | :------: | :-------------------------------------------------------------------------------------------------------------------------------------- |
+| variant | String            | 'normal' | a value of `small` or `normal` results in a smaller or larger version of the StateLabel.                                                |
+| status  | String            |          | Can be one of `issueOpened`, `issueClosed`, `issueClosedNotPlanned`, `pullOpened`, `pullClosed`, `pullMerged`, `draft` or `issueDraft`. |
+| sx      | SystemStyleObject |    {}    | Style to be applied to the component                                                                                                    |
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@primer/react",
-  "version": "35.4.0",
+  "version": "35.5.0",
   "description": "An implementation of GitHub's Primer Design System using React",
   "main": "lib/index.js",
   "module": "lib-esm/index.js",
diff --git a/src/SegmentedControl/SegmentedControl.test.tsx b/src/SegmentedControl/SegmentedControl.test.tsx
@@ -1,26 +1,34 @@
 import React from 'react'
 import '@testing-library/jest-dom/extend-expect'
-import {render} from '@testing-library/react'
+import {fireEvent, render} from '@testing-library/react'
 import {EyeIcon, FileCodeIcon, PeopleIcon} from '@primer/octicons-react'
 import userEvent from '@testing-library/user-event'
 import {behavesAsComponent, checkExports, checkStoriesForAxeViolations} from '../utils/testing'
 import {SegmentedControl} from '.' // TODO: update import when we move this to the global index
 
 const segmentData = [
-  {label: 'Preview', iconLabel: 'EyeIcon', icon: () => <EyeIcon aria-label="EyeIcon" />},
-  {label: 'Raw', iconLabel: 'FileCodeIcon', icon: () => <FileCodeIcon aria-label="FileCodeIcon" />},
-  {label: 'Blame', iconLabel: 'PeopleIcon', icon: () => <PeopleIcon aria-label="PeopleIcon" />}
+  {label: 'Preview', id: 'preview', iconLabel: 'EyeIcon', icon: () => <EyeIcon aria-label="EyeIcon" />},
+  {label: 'Raw', id: 'raw', iconLabel: 'FileCodeIcon', icon: () => <FileCodeIcon aria-label="FileCodeIcon" />},
+  {label: 'Blame', id: 'blame', iconLabel: 'PeopleIcon', icon: () => <PeopleIcon aria-label="PeopleIcon" />}
 ]
 
 // TODO: improve test coverage
 describe('SegmentedControl', () => {
+  const mockWarningFn = jest.fn()
+
+  beforeAll(() => {
+    jest.spyOn(global.console, 'warn').mockImplementation(mockWarningFn)
+  })
+
   behavesAsComponent({
     Component: SegmentedControl,
     toRender: () => (
       <SegmentedControl aria-label="File view">
-        <SegmentedControl.Button selected>Preview</SegmentedControl.Button>
-        <SegmentedControl.Button>Raw</SegmentedControl.Button>
-        <SegmentedControl.Button>Blame</SegmentedControl.Button>
+        {segmentData.map(({label}, index) => (
+          <SegmentedControl.Button selected={index === 0} key={label}>
+            {label}
+          </SegmentedControl.Button>
+        ))}
       </SegmentedControl>
     )
   })
@@ -133,6 +141,68 @@ describe('SegmentedControl', () => {
     }
     expect(handleClick).toHaveBeenCalled()
   })
+
+  it('focuses the selected button first', () => {
+    const {getByRole} = render(
+      <>
+        <button>Before</button>
+        <SegmentedControl aria-label="File view">
+          {segmentData.map(({label, id}, index) => (
+            <SegmentedControl.Button selected={index === 1} key={label} id={id}>
+              {label}
+            </SegmentedControl.Button>
+          ))}
+        </SegmentedControl>
+      </>
+    )
+    const initialFocusButtonNode = getByRole('button', {name: segmentData[1].label})
+
+    expect(document.activeElement?.id).not.toEqual(initialFocusButtonNode.id)
+
+    userEvent.tab() // focus the button before the segmented control
+    userEvent.tab() // move focus into the segmented control
+
+    expect(document.activeElement?.id).toEqual(initialFocusButtonNode.id)
+  })
+
+  it('focuses the previous button when keying ArrowLeft, and the next button when keying ArrowRight', () => {
+    const {getByRole} = render(
+      <SegmentedControl aria-label="File view">
+        {segmentData.map(({label, id}, index) => (
+          <SegmentedControl.Button selected={index === 1} key={label} id={id}>
+            {label}
+          </SegmentedControl.Button>
+        ))}
+      </SegmentedControl>
+    )
+    const initialFocusButtonNode = getByRole('button', {name: segmentData[1].label})
+    const nextFocusButtonNode = getByRole('button', {name: segmentData[0].label})
+
+    expect(document.activeElement?.id).not.toEqual(nextFocusButtonNode.id)
+
+    fireEvent.focus(initialFocusButtonNode)
+    fireEvent.keyDown(initialFocusButtonNode, {key: 'ArrowLeft'})
+
+    expect(document.activeElement?.id).toEqual(nextFocusButtonNode.id)
+
+    fireEvent.keyDown(initialFocusButtonNode, {key: 'ArrowRight'})
+
+    expect(document.activeElement?.id).toEqual(initialFocusButtonNode.id)
+  })
+
+  it('should warn the user if they neglect to specify a label for the segmented control', () => {
+    render(
+      <SegmentedControl>
+        {segmentData.map(({label, id}) => (
+          <SegmentedControl.Button id={id} key={label}>
+            {label}
+          </SegmentedControl.Button>
+        ))}
+      </SegmentedControl>
+    )
+
+    expect(mockWarningFn).toHaveBeenCalled()
+  })
 })
 
 checkStoriesForAxeViolations('examples', '../SegmentedControl/')
diff --git a/src/SegmentedControl/SegmentedControl.tsx b/src/SegmentedControl/SegmentedControl.tsx
@@ -1,8 +1,9 @@
-import React from 'react'
+import React, {RefObject, useRef} from 'react'
 import Button, {SegmentedControlButtonProps} from './SegmentedControlButton'
 import SegmentedControlIconButton, {SegmentedControlIconButtonProps} from './SegmentedControlIconButton'
 import {Box, useTheme} from '..'
 import {merge, SxProp} from '../sx'
+import {FocusKeys, FocusZoneHookSettings, useFocusZone} from '../hooks/useFocusZone'
 
 type SegmentedControlProps = {
   'aria-label'?: string
@@ -28,10 +29,16 @@ const getSegmentedControlStyles = (props?: SegmentedControlProps) => ({
 })
 
 // TODO: implement `variant` prop for responsive behavior
-// TODO: implement `loading` prop
-// TODO: log a warning if no `ariaLabel` or `ariaLabelledBy` prop is passed
-// TODO: implement keyboard behavior to move focus using the arrow keys
-const Root: React.FC<SegmentedControlProps> = ({children, fullWidth, onChange, sx: sxProp = {}, ...rest}) => {
+const Root: React.FC<SegmentedControlProps> = ({
+  'aria-label': ariaLabel,
+  'aria-labelledby': ariaLabelledby,
+  children,
+  fullWidth,
+  onChange,
+  sx: sxProp = {},
+  ...rest
+}) => {
+  const segmentedControlContainerRef = useRef<HTMLSpanElement>(null)
   const {theme} = useTheme()
   const selectedChildren = React.Children.toArray(children).map(
     child =>
@@ -46,8 +53,41 @@ const Root: React.FC<SegmentedControlProps> = ({children, fullWidth, onChange, s
     sxProp as SxProp
   )
 
+  const focusInStrategy: FocusZoneHookSettings['focusInStrategy'] = () => {
+    if (segmentedControlContainerRef.current) {
+      // we need to use type assertion because querySelector returns "Element", not "HTMLElement"
+      type SelectedButton = HTMLButtonElement | undefined
+
+      const selectedButton = segmentedControlContainerRef.current.querySelector(
+        'button[aria-current="true"]'
+      ) as SelectedButton
+
+      return selectedButton
+    }
+  }
+
+  useFocusZone({
+    containerRef: segmentedControlContainerRef,
+    bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd,
+    focusInStrategy
+  })
+
+  if (!ariaLabel && !ariaLabelledby) {
+    // eslint-disable-next-line no-console
+    console.warn(
+      'Use the `aria-label` or `aria-labelledby` prop to provide an accessible label for assistive technology'
+    )
+  }
+
   return (
-    <Box role="toolbar" sx={sx} {...rest}>
+    <Box
+      role="toolbar"
+      sx={sx}
+      aria-label={ariaLabel}
+      aria-labelledby={ariaLabelledby}
+      ref={segmentedControlContainerRef as RefObject<HTMLDivElement>}
+      {...rest}
+    >
       {React.Children.map(children, (child, i) => {
         if (React.isValidElement<SegmentedControlButtonProps | SegmentedControlIconButtonProps>(child)) {
           return React.cloneElement(child, {
diff --git a/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap b/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap
@@ -86,6 +86,10 @@ exports[`SegmentedControl renders consistently 1`] = `
   width: 1px;
 }
 
+.c1:focus:focus-visible:not(:last-child):after {
+  width: 0;
+}
+
 .c1 .segmentedControl-text:after {
   content: "Preview";
   display: block;
@@ -180,6 +184,10 @@ exports[`SegmentedControl renders consistently 1`] = `
   width: 1px;
 }
 
+.c2:focus:focus-visible:not(:last-child):after {
+  width: 0;
+}
+
 .c2 .segmentedControl-text:after {
   content: "Raw";
   display: block;
@@ -274,6 +282,10 @@ exports[`SegmentedControl renders consistently 1`] = `
   width: 1px;
 }
 
+.c3:focus:focus-visible:not(:last-child):after {
+  width: 0;
+}
+
 .c3 .segmentedControl-text:after {
   content: "Blame";
   display: block;
diff --git a/src/SegmentedControl/examples.stories.tsx b/src/SegmentedControl/examples.stories.tsx
@@ -50,7 +50,7 @@ export const Default = (args: Args) => (
 )
 
 export const Controlled = (args: Args) => {
-  const [selectedIndex, setSelectedIndex] = useState(1)
+  const [selectedIndex, setSelectedIndex] = useState(0)
   const handleChange = (i: number) => {
     setSelectedIndex(i)
   }
diff --git a/src/SegmentedControl/getSegmentedControlStyles.ts b/src/SegmentedControl/getSegmentedControlStyles.ts
@@ -75,6 +75,11 @@ const getSegmentedControlButtonStyles = (props?: SegmentedControlButtonProps & {
     }
   },
 
+  // fixes an issue where the focus outline shows over the pseudo-element
+  ':focus:focus-visible:not(:last-child):after': {
+    width: 0
+  },
+
   '.segmentedControl-text': {
     ':after': {
       content: `"${props?.children}"`,
diff --git a/src/StateLabel.tsx b/src/StateLabel.tsx
@@ -2,6 +2,7 @@ import {
   GitMergeIcon,
   GitPullRequestIcon,
   IssueClosedIcon,
+  SkipIcon,
   IssueDraftIcon,
   IssueOpenedIcon,
   QuestionIcon
@@ -18,6 +19,7 @@ const octiconMap = {
   issueOpened: IssueOpenedIcon,
   pullOpened: GitPullRequestIcon,
   issueClosed: IssueClosedIcon,
+  issueClosedNotPlanned: SkipIcon,
   pullClosed: GitPullRequestIcon,
   pullMerged: GitMergeIcon,
   draft: GitPullRequestIcon,
@@ -28,7 +30,11 @@ const colorVariants = variant({
   prop: 'status',
   variants: {
     issueClosed: {
-      backgroundColor: 'danger.emphasis',
+      backgroundColor: 'done.emphasis',
+      color: 'fg.onEmphasis'
+    },
+    issueClosedNotPlanned: {
+      backgroundColor: 'neutral.emphasis',
       color: 'fg.onEmphasis'
     },
     pullClosed: {
diff --git a/src/__tests__/StateLabel.test.tsx b/src/__tests__/StateLabel.test.tsx
@@ -34,6 +34,7 @@ describe('StateLabel', () => {
   it('respects the status prop', () => {
     expect(render(<StateLabel status="issueOpened" />)).toMatchSnapshot()
     expect(render(<StateLabel status="issueClosed" />)).toMatchSnapshot()
+    expect(render(<StateLabel status="issueClosedNotPlanned" />)).toMatchSnapshot()
     expect(render(<StateLabel status="pullMerged" />)).toMatchSnapshot()
   })
 
diff --git a/src/__tests__/__snapshots__/StateLabel.test.tsx.snap b/src/__tests__/__snapshots__/StateLabel.test.tsx.snap

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'@primer/react': patch
 +---
++
 +Support issues closed as not planned, and correct the standard issue closed backgroud colour
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@primer/react",`
`3`		`- "version": "35.4.0",`
	`3`	`+ "version": "35.5.0",`
`4`	`4`	`"description": "An implementation of GitHub's Primer Design System using React",`
`5`	`5`	`"main": "lib/index.js",`
`6`	`6`	`"module": "lib-esm/index.js",`