From dfa027bdc3cc31d3547ed1f9ce1e07408b6e44c1 Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Fri, 5 Aug 2022 14:03:24 -0400 Subject: [PATCH 1/4] updates role and focus behavior for SegmentedControl buttons, fixes style bug for full-width icon-only buttons, fixes bug for icon-only buttons where onClick and other shared props weren't being passed --- src/SegmentedControl/SegmentedControl.tsx | 58 ++++++++----------- .../SegmentedControlButton.tsx | 30 ++++++---- .../SegmentedControlIconButton.tsx | 45 +++++++------- .../getSegmentedControlStyles.ts | 28 +++++---- 4 files changed, 76 insertions(+), 85 deletions(-) diff --git a/src/SegmentedControl/SegmentedControl.tsx b/src/SegmentedControl/SegmentedControl.tsx index 9336c0097dc..ac68861851f 100644 --- a/src/SegmentedControl/SegmentedControl.tsx +++ b/src/SegmentedControl/SegmentedControl.tsx @@ -1,14 +1,19 @@ -import React, {RefObject, useRef} from 'react' +import React, {useRef} from 'react' import Button, {SegmentedControlButtonProps} from './SegmentedControlButton' import SegmentedControlIconButton, {SegmentedControlIconButtonProps} from './SegmentedControlIconButton' -import {ActionList, ActionMenu, Box, useTheme} from '..' -import {merge, SxProp} from '../sx' +import {ActionList, ActionMenu, useTheme} from '..' +import sx, {merge, SxProp} from '../sx' import {ResponsiveValue, useResponsiveValue} from '../hooks/useResponsiveValue' import {ViewportRangeKeys} from '../utils/types/ViewportRangeKeys' -import {FocusKeys, FocusZoneHookSettings, useFocusZone} from '../hooks/useFocusZone' +import styled from 'styled-components' type WidthOnlyViewportRangeKeys = Exclude +// Needed because passing a ref to `Box` causes a type error +const SegmentedControlList = styled.ul` + ${sx}; +` + type SegmentedControlProps = { 'aria-label'?: string 'aria-labelledby'?: string @@ -28,7 +33,9 @@ const getSegmentedControlStyles = (isFullWidth?: boolean) => ({ borderStyle: 'solid', borderWidth: 1, display: isFullWidth ? 'flex' : 'inline-flex', - height: '32px' // TODO: use primitive `control.medium.size` when it is available + margin: 0, + padding: 0, + width: isFullWidth ? '100%' : undefined }) const Root: React.FC> = ({ @@ -41,7 +48,7 @@ const Root: React.FC> = ({ variant, ...rest }) => { - const segmentedControlContainerRef = useRef(null) + const segmentedControlContainerRef = useRef(null) const {theme} = useTheme() const responsiveVariant = useResponsiveValue(variant, 'default') const isFullWidth = useResponsiveValue(fullWidth, false) @@ -74,26 +81,7 @@ const Root: React.FC> = ({ return React.isValidElement(childArg) ? childArg.props['aria-label'] : null } - const sx = merge(getSegmentedControlStyles(isFullWidth), 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 - }) + const listSx = merge(getSegmentedControlStyles(isFullWidth), sxProp as SxProp) if (!ariaLabel && !ariaLabelledby) { // eslint-disable-next-line no-console @@ -141,12 +129,11 @@ const Root: React.FC> = ({ ) : ( // Render a segmented control - } + ref={segmentedControlContainerRef} {...rest} > {React.Children.map(children, (child, index) => { @@ -180,6 +167,7 @@ const Root: React.FC> = ({ children: childPropsChildren, ...restChildProps } = child.props + const {sx: sharedSxProp, ...restSharedChildProps} = sharedChildProps if (!leadingIcon) { // eslint-disable-next-line no-console console.warn('A `leadingIcon` prop is required when hiding visible labels') @@ -190,12 +178,12 @@ const Root: React.FC> = ({ icon={leadingIcon} sx={ { - '--separator-color': - index === selectedIndex || index === selectedIndex - 1 - ? 'transparent' - : theme?.colors.border.default + ...sharedSxProp, + // setting width here avoids having to pass `isFullWidth` directly to child components + width: !isFullWidth ? '32px' : '100%' // TODO: use primitive `control.medium.size` when it is available instead of '32px' } as React.CSSProperties } + {...restSharedChildProps} {...restChildProps} /> ) @@ -205,7 +193,7 @@ const Root: React.FC> = ({ // Render the children as-is and add the shared child props return React.cloneElement(child, sharedChildProps) })} - + ) } diff --git a/src/SegmentedControl/SegmentedControlButton.tsx b/src/SegmentedControl/SegmentedControlButton.tsx index f2054158625..44ec9716a37 100644 --- a/src/SegmentedControl/SegmentedControlButton.tsx +++ b/src/SegmentedControl/SegmentedControlButton.tsx @@ -3,7 +3,7 @@ import {IconProps} from '@primer/octicons-react' import styled from 'styled-components' import {Box} from '..' import sx, {merge, SxProp} from '../sx' -import {getSegmentedControlButtonStyles} from './getSegmentedControlStyles' +import {getSegmentedControlButtonStyles, getSegmentedControlListItemStyles} from './getSegmentedControlStyles' export type SegmentedControlButtonProps = { /** The visible label rendered in the button */ @@ -26,19 +26,25 @@ const SegmentedControlButton: React.FC { - const mergedSx = merge(getSegmentedControlButtonStyles({selected, children}), sxProp as SxProp) + const mergedSx = merge(getSegmentedControlListItemStyles(), sxProp as SxProp) return ( - - - {LeadingIcon && ( - - - - )} - {children} - - + + + + {LeadingIcon && ( + + + + )} + {children} + + + ) } diff --git a/src/SegmentedControl/SegmentedControlIconButton.tsx b/src/SegmentedControl/SegmentedControlIconButton.tsx index 1d617151823..2fd0225c5fc 100644 --- a/src/SegmentedControl/SegmentedControlIconButton.tsx +++ b/src/SegmentedControl/SegmentedControlIconButton.tsx @@ -2,12 +2,9 @@ import React, {HTMLAttributes} from 'react' import {IconProps} from '@primer/octicons-react' import styled from 'styled-components' import sx, {merge, SxProp} from '../sx' -import { - borderedSegment, - getSegmentedControlButtonStyles, - directChildLayoutAdjustments -} from './getSegmentedControlStyles' +import {getSegmentedControlButtonStyles, getSegmentedControlListItemStyles} from './getSegmentedControlStyles' import Tooltip from '../Tooltip' +import Box from '../Box' export type SegmentedControlIconButtonProps = { 'aria-label': string @@ -35,26 +32,28 @@ export const SegmentedControlIconButton: React.FC { - const mergedSx = merge(getSegmentedControlButtonStyles({selected, isIconOnly: true}), sxProp as SxProp) + const mergedSx = merge( + { + width: '32px', // TODO: use primitive `control.medium.size` when it is available + ...getSegmentedControlListItemStyles() + }, + sxProp as SxProp + ) return ( - - - - - - - + + + + + + + + + ) } diff --git a/src/SegmentedControl/getSegmentedControlStyles.ts b/src/SegmentedControl/getSegmentedControlStyles.ts index bac77d60a1d..35b8f5a3445 100644 --- a/src/SegmentedControl/getSegmentedControlStyles.ts +++ b/src/SegmentedControl/getSegmentedControlStyles.ts @@ -36,18 +36,12 @@ export const getSegmentedControlButtonStyles = ( borderWidth: 0, color: 'currentColor', cursor: 'pointer', - flexGrow: 1, fontFamily: 'inherit', fontSize: 1, fontWeight: props?.selected ? 'bold' : 'normal', - marginTop: '-1px', - marginBottom: '-1px', padding: props?.selected ? 0 : 'var(--segmented-control-button-bg-inset)', - position: 'relative', - ...(props?.isIconOnly && { - height: '32px', // TODO: use primitive `control.medium.size` when it is available - width: '32px' // TODO: use primitive `control.medium.size` when it is available - }), + height: '100%', + width: '100%', '.segmentedControl-content': { alignItems: 'center', @@ -83,13 +77,6 @@ export const getSegmentedControlButtonStyles = ( backgroundColor: props?.selected ? undefined : 'segmentedControl.button.active.bg' }, - // Icon-only buttons render the button inside of an element rendered by the Tooltip component. - // This breaks `:first-child` and `:last-child` selectors on buttons because the button is the only child inside of the element rendered by the Tooltip component. - ...(!props?.isIconOnly && { - ...directChildLayoutAdjustments, - ':not(:last-child)': borderedSegment - }), - // fixes an issue where the focus outline shows over the pseudo-element ':focus:focus-visible:not(:last-child):after': { width: 0 @@ -120,3 +107,14 @@ export const getSegmentedControlButtonStyles = ( } } }) + +export const getSegmentedControlListItemStyles = () => ({ + display: 'block', + position: 'relative', + flexGrow: 1, + marginTop: '-1px', + marginBottom: '-1px', + height: '32px', // TODO: use primitive `control.medium.size` when it is available + ':not(:last-child)': borderedSegment, + ...directChildLayoutAdjustments +}) From 7d1766be8f728db2d844038ef529b34b28ef855c Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Fri, 5 Aug 2022 14:13:15 -0400 Subject: [PATCH 2/4] updates tests --- .../SegmentedControl.test.tsx | 49 --- .../SegmentedControl.test.tsx.snap | 310 +++++++++--------- 2 files changed, 154 insertions(+), 205 deletions(-) diff --git a/src/SegmentedControl/SegmentedControl.test.tsx b/src/SegmentedControl/SegmentedControl.test.tsx index 3a0a094b008..1129507e80e 100644 --- a/src/SegmentedControl/SegmentedControl.test.tsx +++ b/src/SegmentedControl/SegmentedControl.test.tsx @@ -197,55 +197,6 @@ describe('SegmentedControl', () => { expect(handleClick).toHaveBeenCalled() }) - it('focuses the selected button first', async () => { - const user = userEvent.setup() - const {getByRole} = render( - <> - - - {segmentData.map(({label, id}, index) => ( - - {label} - - ))} - - - ) - const initialFocusButtonNode = getByRole('button', {name: segmentData[1].label}) - - expect(document.activeElement?.id).not.toEqual(initialFocusButtonNode.id) - - await user.tab() // focus the button before the segmented control - await user.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( - - {segmentData.map(({label, id}, index) => ( - - {label} - - ))} - - ) - 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('calls onChange with index of clicked segment button when using the dropdown variant', async () => { act(() => { matchMedia.useMediaQuery(viewportRanges.narrow) diff --git a/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap b/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap index fd92c0bfb14..f5dfa8a4557 100644 --- a/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap +++ b/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap @@ -1,20 +1,77 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`SegmentedControl renders consistently 1`] = ` -.c0 { - background-color: #eaeef2; - border-color: #d0d7de; - border-radius: 6px; - border-style: solid; - border-width: 1px; - display: -webkit-inline-box; - display: -webkit-inline-flex; - display: -ms-inline-flexbox; - display: inline-flex; +.c1 { + display: block; + position: relative; + -webkit-box-flex: 1; + -webkit-flex-grow: 1; + -ms-flex-positive: 1; + flex-grow: 1; + margin-top: -1px; + margin-bottom: -1px; height: 32px; + --separator-color: transparent; } -.c1 { +.c1:not(:last-child) { + margin-right: 1px; +} + +.c1:not(:last-child):after { + background-color: var(--separator-color); + content: ""; + position: absolute; + right: -2px; + top: 8px; + bottom: 8px; + width: 1px; +} + +.c1:first-child { + margin-left: -1px; +} + +.c1:last-child { + margin-right: -1px; +} + +.c3 { + display: block; + position: relative; + -webkit-box-flex: 1; + -webkit-flex-grow: 1; + -ms-flex-positive: 1; + flex-grow: 1; + margin-top: -1px; + margin-bottom: -1px; + height: 32px; + --separator-color: #d0d7de; +} + +.c3:not(:last-child) { + margin-right: 1px; +} + +.c3:not(:last-child):after { + background-color: var(--separator-color); + content: ""; + position: absolute; + right: -2px; + top: 8px; + bottom: 8px; + width: 1px; +} + +.c3:first-child { + margin-left: -1px; +} + +.c3:last-child { + margin-right: -1px; +} + +.c2 { --segmented-control-button-inner-padding: 12px; --segmented-control-button-bg-inset: 4px; --segmented-control-outer-radius: 6px; @@ -24,21 +81,15 @@ exports[`SegmentedControl renders consistently 1`] = ` border-width: 0; color: currentColor; cursor: pointer; - -webkit-box-flex: 1; - -webkit-flex-grow: 1; - -ms-flex-positive: 1; - flex-grow: 1; font-family: inherit; font-size: 14px; font-weight: 600; - margin-top: -1px; - margin-bottom: -1px; padding: 0; - position: relative; - --separator-color: transparent; + height: 100%; + width: 100%; } -.c1 .segmentedControl-content { +.c2 .segmentedControl-content { -webkit-align-items: center; -webkit-box-align: center; -ms-flex-align: center; @@ -61,37 +112,15 @@ exports[`SegmentedControl renders consistently 1`] = ` padding-right: var(--segmented-control-button-inner-padding); } -.c1 svg { +.c2 svg { fill: #57606a; } -.c1:first-child { - margin-left: -1px; -} - -.c1:last-child { - margin-right: -1px; -} - -.c1:not(:last-child) { - margin-right: 1px; -} - -.c1:not(:last-child):after { - background-color: var(--separator-color); - content: ""; - position: absolute; - right: -2px; - top: 8px; - bottom: 8px; - width: 1px; -} - -.c1:focus:focus-visible:not(:last-child):after { +.c2:focus:focus-visible:not(:last-child):after { width: 0; } -.c1 .segmentedControl-text:after { +.c2 .segmentedControl-text:after { content: "Preview"; display: block; font-weight: 600; @@ -105,7 +134,7 @@ exports[`SegmentedControl renders consistently 1`] = ` visibility: hidden; } -.c2 { +.c4 { --segmented-control-button-inner-padding: 12px; --segmented-control-button-bg-inset: 4px; --segmented-control-outer-radius: 6px; @@ -115,21 +144,15 @@ exports[`SegmentedControl renders consistently 1`] = ` border-width: 0; color: currentColor; cursor: pointer; - -webkit-box-flex: 1; - -webkit-flex-grow: 1; - -ms-flex-positive: 1; - flex-grow: 1; font-family: inherit; font-size: 14px; font-weight: 400; - margin-top: -1px; - margin-bottom: -1px; padding: var(--segmented-control-button-bg-inset); - position: relative; - --separator-color: #d0d7de; + height: 100%; + width: 100%; } -.c2 .segmentedControl-content { +.c4 .segmentedControl-content { -webkit-align-items: center; -webkit-box-align: center; -ms-flex-align: center; @@ -152,45 +175,23 @@ exports[`SegmentedControl renders consistently 1`] = ` padding-right: calc(var(--segmented-control-button-inner-padding) - var(--segmented-control-button-bg-inset)); } -.c2 svg { +.c4 svg { fill: #57606a; } -.c2:hover .segmentedControl-content { +.c4:hover .segmentedControl-content { background-color: rgba(175,184,193,0.2); } -.c2:active .segmentedControl-content { +.c4:active .segmentedControl-content { background-color: rgba(175,184,193,0.4); } -.c2:first-child { - margin-left: -1px; -} - -.c2:last-child { - margin-right: -1px; -} - -.c2:not(:last-child) { - margin-right: 1px; -} - -.c2:not(:last-child):after { - background-color: var(--separator-color); - content: ""; - position: absolute; - right: -2px; - top: 8px; - bottom: 8px; - width: 1px; -} - -.c2:focus:focus-visible:not(:last-child):after { +.c4:focus:focus-visible:not(:last-child):after { width: 0; } -.c2 .segmentedControl-text:after { +.c4 .segmentedControl-text:after { content: "Raw"; display: block; font-weight: 600; @@ -204,7 +205,7 @@ exports[`SegmentedControl renders consistently 1`] = ` visibility: hidden; } -.c3 { +.c5 { --segmented-control-button-inner-padding: 12px; --segmented-control-button-bg-inset: 4px; --segmented-control-outer-radius: 6px; @@ -214,21 +215,15 @@ exports[`SegmentedControl renders consistently 1`] = ` border-width: 0; color: currentColor; cursor: pointer; - -webkit-box-flex: 1; - -webkit-flex-grow: 1; - -ms-flex-positive: 1; - flex-grow: 1; font-family: inherit; font-size: 14px; font-weight: 400; - margin-top: -1px; - margin-bottom: -1px; padding: var(--segmented-control-button-bg-inset); - position: relative; - --separator-color: #d0d7de; + height: 100%; + width: 100%; } -.c3 .segmentedControl-content { +.c5 .segmentedControl-content { -webkit-align-items: center; -webkit-box-align: center; -ms-flex-align: center; @@ -251,45 +246,23 @@ exports[`SegmentedControl renders consistently 1`] = ` padding-right: calc(var(--segmented-control-button-inner-padding) - var(--segmented-control-button-bg-inset)); } -.c3 svg { +.c5 svg { fill: #57606a; } -.c3:hover .segmentedControl-content { +.c5:hover .segmentedControl-content { background-color: rgba(175,184,193,0.2); } -.c3:active .segmentedControl-content { +.c5:active .segmentedControl-content { background-color: rgba(175,184,193,0.4); } -.c3:first-child { - margin-left: -1px; -} - -.c3:last-child { - margin-right: -1px; -} - -.c3:not(:last-child) { - margin-right: 1px; -} - -.c3:not(:last-child):after { - background-color: var(--separator-color); - content: ""; - position: absolute; - right: -2px; - top: 8px; - bottom: 8px; - width: 1px; -} - -.c3:focus:focus-visible:not(:last-child):after { +.c5:focus:focus-visible:not(:last-child):after { width: 0; } -.c3 .segmentedControl-text:after { +.c5 .segmentedControl-text:after { content: "Blame"; display: block; font-weight: 600; @@ -303,8 +276,22 @@ exports[`SegmentedControl renders consistently 1`] = ` visibility: hidden; } +.c0 { + background-color: #eaeef2; + border-color: #d0d7de; + border-radius: 6px; + border-style: solid; + border-width: 1px; + display: -webkit-inline-box; + display: -webkit-inline-flex; + display: -ms-inline-flexbox; + display: inline-flex; + margin: 0; + padding: 0; +} + @media (pointer:coarse) { - .c1:before { + .c2:before { content: ""; position: absolute; left: 0; @@ -318,7 +305,7 @@ exports[`SegmentedControl renders consistently 1`] = ` } @media (pointer:coarse) { - .c2:before { + .c4:before { content: ""; position: absolute; left: 0; @@ -332,7 +319,7 @@ exports[`SegmentedControl renders consistently 1`] = ` } @media (pointer:coarse) { - .c3:before { + .c5:before { content: ""; position: absolute; left: 0; @@ -345,52 +332,63 @@ exports[`SegmentedControl renders consistently 1`] = ` } } -
- -
+ + + +
  • - -
    - Raw -
    -     - - +
    • +
  • - -
    - Blame -
    -     - - +
    + Blame +
    + + +
    • + `; From e2749b04b2217ce3598c1b6503ef102cd7bc7263 Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Fri, 5 Aug 2022 14:39:56 -0400 Subject: [PATCH 3/4] adds changeset --- .changeset/old-experts-applaud.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/old-experts-applaud.md diff --git a/.changeset/old-experts-applaud.md b/.changeset/old-experts-applaud.md new file mode 100644 index 00000000000..12c5b68ed33 --- /dev/null +++ b/.changeset/old-experts-applaud.md @@ -0,0 +1,7 @@ +--- +'@primer/react': patch +--- + +- Fixes `role` and keyboard behavior for SegmentedControl. +- Fixes a bug where icon-only SegmentedControl buttons did not fill the parent width when the `fullWidth` prop was set +- Fixes a bug where click handlers were not passed correctly when the responsive variant was set to `'hideLabels'` From c7567a0e023c713fd1fd649be80a92d4cd7346cf Mon Sep 17 00:00:00 2001 From: Mike Perrotti Date: Fri, 5 Aug 2022 14:57:59 -0400 Subject: [PATCH 4/4] fixes visual regression --- src/SegmentedControl/SegmentedControl.tsx | 1 + .../__snapshots__/SegmentedControl.test.tsx.snap | 1 + 2 files changed, 2 insertions(+) diff --git a/src/SegmentedControl/SegmentedControl.tsx b/src/SegmentedControl/SegmentedControl.tsx index ac68861851f..2a7e054336e 100644 --- a/src/SegmentedControl/SegmentedControl.tsx +++ b/src/SegmentedControl/SegmentedControl.tsx @@ -33,6 +33,7 @@ const getSegmentedControlStyles = (isFullWidth?: boolean) => ({ borderStyle: 'solid', borderWidth: 1, display: isFullWidth ? 'flex' : 'inline-flex', + height: '32px', // TODO: use primitive `control.medium.size` when it is available margin: 0, padding: 0, width: isFullWidth ? '100%' : undefined diff --git a/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap b/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap index f5dfa8a4557..0dd5f099183 100644 --- a/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap +++ b/src/SegmentedControl/__snapshots__/SegmentedControl.test.tsx.snap @@ -286,6 +286,7 @@ exports[`SegmentedControl renders consistently 1`] = ` display: -webkit-inline-flex; display: -ms-inline-flexbox; display: inline-flex; + height: 32px; margin: 0; padding: 0; }