Skip to content

Commit

Permalink
refactor(ButtonBase): To use CSS modules behind the feature flag (#4940)
Browse files Browse the repository at this point in the history
* copy

* setting

* Create olive-donkeys-exercise.md

* test(vrt): update snapshots

* update tests

* we got this

* vrt dont lie

* test(vrt): update snapshots

* more fixes

* fix focus outline

* where where where

* fix nesting

* fix nesting i think

* test vrt

* cleanup

* important...

* try reverting this change?

* try resetting stories

* reset from main

* fix classname on iconbutton

* try no calc

* more digits??

* test(vrt): update snapshots

* lets goooooo

* oops

* reset

* Revert "reset"

This reverts commit b82f00c.

* reset

* test(vrt): update snapshots

---------

Co-authored-by: langermank <langermank@users.noreply.github.com>
langermank and langermank authored Sep 13, 2024
1 parent 6e294e8 commit 4d3b504
Showing 14 changed files with 1,670 additions and 716 deletions.
5 changes: 5 additions & 0 deletions .changeset/olive-donkeys-exercise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": major
---

Refactor ButtonBase component to use CSS modules behine flag
7 changes: 7 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -10,6 +10,13 @@
"[typescriptreact]": {
"editor.defaultFormatter": "esbenp.prettier-vscode"
},
"[postcss]": {
"editor.defaultFormatter": "esbenp.prettier-vscode",
"editor.formatOnSave": true,
"editor.codeActionsOnSave": {
"source.fixAll.stylelint": "explicit"
}
},
"json.schemas": [
{
"fileMatch": ["*.docs.json"],
744 changes: 407 additions & 337 deletions e2e/components/IconButton.test.ts

Large diffs are not rendered by default.

774 changes: 422 additions & 352 deletions e2e/components/LinkButton.test.ts

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions packages/react/.storybook/storybook.css
Original file line number Diff line number Diff line change
@@ -54,3 +54,7 @@
font: var(--text-caption-shorthand);
margin: 0;
}

.testCustomClassname {
color: var(--fgColor-sponsors);
}
501 changes: 501 additions & 0 deletions packages/react/src/Button/ButtonBase.module.css

Large diffs are not rendered by default.

248 changes: 248 additions & 0 deletions packages/react/src/Button/ButtonBase.tsx
Original file line number Diff line number Diff line change
@@ -16,6 +16,9 @@ import CounterLabel from '../CounterLabel'
import {useId} from '../hooks'
import {ConditionalWrapper} from '../internal/components/ConditionalWrapper'
import {AriaStatus} from '../live-region'
import {clsx} from 'clsx'
import classes from './ButtonBase.module.css'
import {useFeatureFlag} from '../FeatureFlags'

const iconWrapStyles = {
display: 'flex',
@@ -28,6 +31,15 @@ const renderVisual = (Visual: React.ElementType, loading: boolean, visualName: s
</Box>
)

const renderModuleVisual = (Visual: React.ElementType, loading: boolean, visualName: string, counterLabel: boolean) => (
<span
data-component={visualName}
className={clsx(!counterLabel && classes.Visual, loading ? classes.loadingSpinner : classes.VisualWrap)}
>
{loading ? <Spinner size="small" /> : <Visual />}
</span>
)

const ButtonBase = forwardRef(
({children, as: Component = 'button', sx: sxProp = defaultSxProp, ...props}, forwardedRef): JSX.Element => {
const {
@@ -48,9 +60,11 @@ const ButtonBase = forwardRef(
inactive,
onClick,
labelWrap,
className,
...rest
} = props

const enabled = useFeatureFlag('primer_react_css_modules_team')
const innerRef = React.useRef<HTMLButtonElement>(null)
useRefObjectAsForwardedRef(forwardedRef, innerRef)

@@ -84,6 +98,239 @@ const ButtonBase = forwardRef(
}, [innerRef])
}

if (enabled) {
if (sxProp !== defaultSxProp) {
return (
<ConditionalWrapper
// If anything is passsed to `loading`, we need the wrapper:
// If we just checked for `loading` as a boolean, the wrapper wouldn't be rendered
// when `loading` is `false`.
// Then, the component re-renders in a way that the button will lose focus when switching between loading states.
if={typeof loading !== 'undefined'}
className={block ? classes.ConditionalWrapper : undefined}
data-loading-wrapper
>
<Box
as={Component}
sx={sxStyles}
aria-disabled={loading ? true : undefined}
{...rest}
ref={innerRef}
className={clsx(classes.ButtonBase, className)}
data-block={block ? 'block' : null}
data-inactive={inactive ? true : undefined}
data-loading={Boolean(loading)}
data-no-visuals={!LeadingVisual && !TrailingVisual && !TrailingAction ? true : undefined}
data-size={size}
data-variant={variant}
data-label-wrap={labelWrap}
aria-describedby={[loadingAnnouncementID, ariaDescribedBy]
.filter(descriptionID => Boolean(descriptionID))
.join(' ')}
// aria-labelledby is needed because the accessible name becomes unset when the button is in a loading state.
// We only set it when the button is in a loading state because it will supercede the aria-label when the screen
// reader announces the button name.
aria-labelledby={
loading
? [`${uuid}-label`, ariaLabelledBy].filter(labelID => Boolean(labelID)).join(' ')
: ariaLabelledBy
}
id={id}
onClick={loading ? undefined : onClick}
>
{Icon ? (
loading ? (
<Spinner size="small" />
) : (
<Icon />
)
) : (
<>
<Box
as="span"
data-component="buttonContent"
sx={getAlignContentSize(alignContent)}
className={classes.ButtonContent}
>
{
/* If there are no leading/trailing visuals/actions to replace with a loading spinner,
render a loading spiner in place of the button content. */
loading &&
!LeadingVisual &&
!TrailingVisual &&
!TrailingAction &&
renderModuleVisual(Spinner, loading, 'loadingSpinner', false)
}
{
/* Render a leading visual unless the button is in a loading state.
Then replace the leading visual with a loading spinner. */
LeadingVisual && renderModuleVisual(LeadingVisual, Boolean(loading), 'leadingVisual', false)
}
{children && (
<span data-component="text" className={classes.Label} id={loading ? `${uuid}-label` : undefined}>
{children}
</span>
)}
{
/* If there is a count, render a counter label unless there is a trailing visual.
Then render the counter label as a trailing visual.
Replace the counter label or the trailing visual with a loading spinner if:
- the button is in a loading state
- there is no leading visual to replace with a loading spinner
*/
count !== undefined && !TrailingVisual
? renderModuleVisual(
() => (
<CounterLabel className={classes.CounterLabel} data-component="ButtonCounter">
{count}
</CounterLabel>
),
Boolean(loading) && !LeadingVisual,
'trailingVisual',
true,
)
: TrailingVisual
? renderModuleVisual(
TrailingVisual,
Boolean(loading) && !LeadingVisual,
'trailingVisual',
false,
)
: null
}
</Box>
{
/* If there is a trailing action, render it unless the button is in a loading state
and there is no leading or trailing visual to replace with a loading spinner. */
TrailingAction &&
renderModuleVisual(
TrailingAction,
Boolean(loading) && !LeadingVisual && !TrailingVisual,
'trailingAction',
false,
)
}
</>
)}
</Box>
{loading && (
<VisuallyHidden>
<AriaStatus id={loadingAnnouncementID}>{loadingAnnouncement}</AriaStatus>
</VisuallyHidden>
)}
</ConditionalWrapper>
)
}
return (
<ConditionalWrapper
// If anything is passsed to `loading`, we need the wrapper:
// If we just checked for `loading` as a boolean, the wrapper wouldn't be rendered
// when `loading` is `false`.
// Then, the component re-renders in a way that the button will lose focus when switching between loading states.
if={typeof loading !== 'undefined'}
className={block ? classes.ConditionalWrapper : undefined}
data-loading-wrapper
>
<Component
aria-disabled={loading ? true : undefined}
{...rest}
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
ref={innerRef}
className={clsx(classes.ButtonBase, className)}
data-block={block ? 'block' : null}
data-inactive={inactive ? true : undefined}
data-loading={Boolean(loading)}
data-no-visuals={!LeadingVisual && !TrailingVisual && !TrailingAction ? true : undefined}
data-size={size}
data-variant={variant}
data-label-wrap={labelWrap}
aria-describedby={[loadingAnnouncementID, ariaDescribedBy]
.filter(descriptionID => Boolean(descriptionID))
.join(' ')}
// aria-labelledby is needed because the accessible name becomes unset when the button is in a loading state.
// We only set it when the button is in a loading state because it will supercede the aria-label when the screen
// reader announces the button name.
aria-labelledby={
loading ? [`${uuid}-label`, ariaLabelledBy].filter(labelID => Boolean(labelID)).join(' ') : ariaLabelledBy
}
id={id}
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
onClick={loading ? undefined : onClick}
>
{Icon ? (
loading ? (
<Spinner size="small" />
) : (
<Icon />
)
) : (
<>
<span data-component="buttonContent" data-align={alignContent} className={classes.ButtonContent}>
{
/* If there are no leading/trailing visuals/actions to replace with a loading spinner,
render a loading spiner in place of the button content. */
loading &&
!LeadingVisual &&
!TrailingVisual &&
!TrailingAction &&
renderModuleVisual(Spinner, loading, 'loadingSpinner', false)
}
{
/* Render a leading visual unless the button is in a loading state.
Then replace the leading visual with a loading spinner. */
LeadingVisual && renderModuleVisual(LeadingVisual, Boolean(loading), 'leadingVisual', false)
}
{children && (
<span data-component="text" className={classes.Label} id={loading ? `${uuid}-label` : undefined}>
{children}
</span>
)}
{
/* If there is a count, render a counter label unless there is a trailing visual.
Then render the counter label as a trailing visual.
Replace the counter label or the trailing visual with a loading spinner if:
- the button is in a loading state
- there is no leading visual to replace with a loading spinner
*/
count !== undefined && !TrailingVisual
? renderModuleVisual(
() => (
<CounterLabel className={classes.CounterLabel} data-component="ButtonCounter">
{count}
</CounterLabel>
),
Boolean(loading) && !LeadingVisual,
'trailingVisual',
true,
)
: TrailingVisual
? renderModuleVisual(TrailingVisual, Boolean(loading) && !LeadingVisual, 'trailingVisual', false)
: null
}
</span>
{
/* If there is a trailing action, render it unless the button is in a loading state
and there is no leading or trailing visual to replace with a loading spinner. */
TrailingAction &&
renderModuleVisual(
TrailingAction,
Boolean(loading) && !LeadingVisual && !TrailingVisual,
'trailingAction',
false,
)
}
</>
)}
</Component>
{loading && (
<VisuallyHidden>
<AriaStatus id={loadingAnnouncementID}>{loadingAnnouncement}</AriaStatus>
</VisuallyHidden>
)}
</ConditionalWrapper>
)
}

return (
<ConditionalWrapper
// If anything is passsed to `loading`, we need the wrapper:
@@ -100,6 +347,7 @@ const ButtonBase = forwardRef(
aria-disabled={loading ? true : undefined}
{...rest}
ref={innerRef}
className={className}
data-block={block ? 'block' : null}
data-inactive={inactive ? true : undefined}
data-loading={Boolean(loading)}
5 changes: 5 additions & 0 deletions packages/react/src/Button/IconButton.tsx
Original file line number Diff line number Diff line change
@@ -6,6 +6,8 @@ import {defaultSxProp} from '../utils/defaultSxProp'
import {generateCustomSxProp} from './Button'
import {TooltipContext, Tooltip} from '../TooltipV2/Tooltip'
import {TooltipContext as TooltipContextV1} from '../Tooltip/Tooltip'
import classes from './ButtonBase.module.css'
import {clsx} from 'clsx'

const IconButton = forwardRef(
(
@@ -19,6 +21,7 @@ const IconButton = forwardRef(
// This is planned to be a temporary prop until the default tooltip on icon buttons are fully rolled out.
unsafeDisableTooltip = false,
keyshortcuts,
className,
...props
},
forwardedRef,
@@ -43,6 +46,7 @@ const IconButton = forwardRef(
return (
<ButtonBase
icon={Icon}
className={clsx(className, classes.IconButton)}
data-component="IconButton"
sx={sxStyles}
type="button"
@@ -66,6 +70,7 @@ const IconButton = forwardRef(
>
<ButtonBase
icon={Icon}
className={clsx(className, classes.IconButton)}
data-component="IconButton"
sx={sxStyles}
type="button"
44 changes: 43 additions & 1 deletion packages/react/src/Button/__tests__/Button.test.tsx
Original file line number Diff line number Diff line change
@@ -2,9 +2,10 @@ import {SearchIcon, HeartIcon} from '@primer/octicons-react'
import {render, screen, fireEvent} from '@testing-library/react'
import axe from 'axe-core'
import React from 'react'
import {IconButton, Button} from '../../Button'
import {IconButton, Button, LinkButton} from '../../Button'
import type {ButtonProps} from '../../Button'
import {behavesAsComponent} from '../../utils/testing'
import {FeatureFlags} from '../../FeatureFlags'

type StatefulLoadingButtonProps = {
children?: React.ReactNode
@@ -300,4 +301,45 @@ describe('Button', () => {
expect(tooltipEl).toBeInTheDocument()
expect(triggerEl.getAttribute('aria-describedby')).toEqual(expect.stringContaining(tooltipEl.id))
})

describe('with primer_react_css_modules_staff enabled', () => {
it('iconbutton should support custom `className` along with default classnames', () => {
const {container} = render(
<FeatureFlags
flags={{
primer_react_css_modules_team: true,
}}
>
<IconButton className="test" aria-label="Test" icon={HeartIcon} />
</FeatureFlags>,
)
expect(container.firstChild).toHaveClass('IconButton')
})

it('button should support custom `className` along with default classnames', () => {
const {container} = render(
<FeatureFlags
flags={{
primer_react_css_modules_team: true,
}}
>
<Button className="test">Hello</Button>
</FeatureFlags>,
)
expect(container.firstChild).toHaveClass('ButtonBase')
})

it('linkbutton should support custom `className` along with default classnames', () => {
const {container} = render(
<FeatureFlags
flags={{
primer_react_css_modules_team: true,
}}
>
<LinkButton className="test">Hello</LinkButton>
</FeatureFlags>,
)
expect(container.firstChild).toHaveClass('ButtonBase')
})
})
})
Original file line number Diff line number Diff line change
@@ -128,7 +128,7 @@ exports[`Button respects block prop 1`] = `
}
.c0[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c0[data-size="small"] [data-component=ButtonCounter] {
@@ -221,7 +221,7 @@ exports[`Button respects block prop 1`] = `
.c0 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
@@ -445,7 +445,7 @@ exports[`Button respects the alignContent prop 1`] = `
}
.c0[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c0[data-size="small"] [data-component=ButtonCounter] {
@@ -538,7 +538,7 @@ exports[`Button respects the alignContent prop 1`] = `
.c0 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
@@ -761,7 +761,7 @@ exports[`Button respects the large size prop 1`] = `
}
.c0[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c0[data-size="small"] [data-component=ButtonCounter] {
@@ -854,7 +854,7 @@ exports[`Button respects the large size prop 1`] = `
.c0 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
@@ -1078,7 +1078,7 @@ exports[`Button respects the small size prop 1`] = `
}
.c0[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c0[data-size="small"] [data-component=ButtonCounter] {
@@ -1171,7 +1171,7 @@ exports[`Button respects the small size prop 1`] = `
.c0 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
@@ -1397,7 +1397,7 @@ exports[`Button styles danger button appropriately 1`] = `
}
.c0[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c0[data-size="small"] [data-component=ButtonCounter] {
@@ -1490,7 +1490,7 @@ exports[`Button styles danger button appropriately 1`] = `
.c0 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
@@ -1714,7 +1714,7 @@ exports[`Button styles invisible button appropriately 1`] = `
}
.c0[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c0[data-size="small"] [data-component=ButtonCounter] {
@@ -1808,7 +1808,7 @@ exports[`Button styles invisible button appropriately 1`] = `
.c0 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
@@ -2041,7 +2041,7 @@ exports[`Button styles primary button appropriately 1`] = `
}
.c0[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c0[data-size="small"] [data-component=ButtonCounter] {
@@ -2134,7 +2134,7 @@ exports[`Button styles primary button appropriately 1`] = `
.c0 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
4 changes: 2 additions & 2 deletions packages/react/src/Button/styles.ts
Original file line number Diff line number Diff line change
@@ -299,7 +299,7 @@ export const getBaseStyles = (theme?: Theme) => ({
fontSize: '0',

'[data-component="text"]': {
lineHeight: 'calc(20 / 12)',
lineHeight: '1.6666667',
},

'[data-component=ButtonCounter]': {
@@ -385,7 +385,7 @@ export const getButtonStyles = (theme?: Theme) => {
},
'[data-component="text"]': {
gridArea: 'text',
lineHeight: 'calc(20/14)',
lineHeight: '1.4285714',
whiteSpace: 'nowrap',
},
'[data-component="trailingVisual"]': {
4 changes: 3 additions & 1 deletion packages/react/src/CounterLabel/CounterLabel.tsx
Original file line number Diff line number Diff line change
@@ -9,15 +9,17 @@ import {defaultSxProp} from '../utils/defaultSxProp'
export type CounterLabelProps = React.PropsWithChildren<
HTMLAttributes<HTMLSpanElement> & {
scheme?: 'primary' | 'secondary'
className?: string
} & SxProp
>

const CounterLabel = forwardRef<HTMLSpanElement, CounterLabelProps>(
({scheme = 'secondary', sx = defaultSxProp, children, ...props}, forwardedRef) => {
({scheme = 'secondary', sx = defaultSxProp, children, className, ...props}, forwardedRef) => {
return (
<>
<Box
aria-hidden="true"
className={className}
sx={merge<BetterSystemStyleObject>(
{
display: 'inline-block',
4 changes: 2 additions & 2 deletions packages/react/src/Heading/Heading.tsx
Original file line number Diff line number Diff line change
@@ -65,7 +65,7 @@ const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props}
className={clsx(className, classes.Heading)}
data-variant={variant}
{...props}
// @ts-ignore shh
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
ref={innerRef}
/>
)
@@ -80,7 +80,7 @@ const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props}
data-variant={variant}
sx={sx}
{...props}
// @ts-ignore shh
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
ref={innerRef}
/>
)
Original file line number Diff line number Diff line change
@@ -1790,7 +1790,7 @@ exports[`TextInput renders trailingAction icon button 1`] = `
}
.c4[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c4[data-size="small"] [data-component=ButtonCounter] {
@@ -1884,7 +1884,7 @@ exports[`TextInput renders trailingAction icon button 1`] = `
.c4 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
@@ -2240,7 +2240,7 @@ exports[`TextInput renders trailingAction icon button 1`] = `
<button
aria-describedby=":ro:-loading-announcement"
aria-labelledby=":rn:"
className="c4"
className="c4 IconButton"
data-block={null}
data-component="IconButton"
data-loading={false}
@@ -2423,7 +2423,7 @@ exports[`TextInput renders trailingAction text button 1`] = `
}
.c4[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c4[data-size="small"] [data-component=ButtonCounter] {
@@ -2517,7 +2517,7 @@ exports[`TextInput renders trailingAction text button 1`] = `
.c4 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}
@@ -2917,7 +2917,7 @@ exports[`TextInput renders trailingAction text button with a tooltip 1`] = `
}
.c4[data-size="small"] [data-component="text"] {
line-height: calc(20 / 12);
line-height: 1.6666667;
}
.c4[data-size="small"] [data-component=ButtonCounter] {
@@ -3011,7 +3011,7 @@ exports[`TextInput renders trailingAction text button with a tooltip 1`] = `
.c4 [data-component="text"] {
grid-area: text;
line-height: calc(20/14);
line-height: 1.4285714;
white-space: nowrap;
}

0 comments on commit 4d3b504

Please sign in to comment.