From 43c06afbe9b5cfa018257335aff7f620be69b1cc Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Mon, 26 Aug 2024 22:25:47 +0000 Subject: [PATCH 01/11] Import postcsspresetprimer to storybook config --- packages/react/postcss.config.js | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/packages/react/postcss.config.js b/packages/react/postcss.config.js index afa4547731c..d6c4ad59862 100644 --- a/packages/react/postcss.config.js +++ b/packages/react/postcss.config.js @@ -1,13 +1,4 @@ +import postcssPresetPrimer from 'postcss-preset-primer' + /** @type {import('postcss-load-config').Config} */ -module.exports = { - plugins: [ - require('postcss-preset-env')({ - stage: 2, // https://preset-env.cssdb.org/features/#stage-2 - features: { - 'nesting-rules': {noIsPseudoSelector: true}, - 'focus-visible-pseudo-class': false, - 'logical-properties-and-values': false, - }, - }), - ], -} +module.exports = postcssPresetPrimer From 25d3da5c8db3e1d181929c171434ce8bc116a24b Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Mon, 26 Aug 2024 22:26:10 +0000 Subject: [PATCH 02/11] Converting to CSS module --- .../src/Avatar/Avatar.features.stories.tsx | 10 ++++ packages/react/src/Avatar/Avatar.module.css | 31 ++++++++++ packages/react/src/Avatar/Avatar.tsx | 60 ++++++++++++++++++- 3 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 packages/react/src/Avatar/Avatar.module.css diff --git a/packages/react/src/Avatar/Avatar.features.stories.tsx b/packages/react/src/Avatar/Avatar.features.stories.tsx index d9d85814c15..dc5ee4b0636 100644 --- a/packages/react/src/Avatar/Avatar.features.stories.tsx +++ b/packages/react/src/Avatar/Avatar.features.stories.tsx @@ -9,6 +9,16 @@ export default { export const Square = () => +export const SquareSxProp = () => ( + +) + export const Size = () => (
diff --git a/packages/react/src/Avatar/Avatar.module.css b/packages/react/src/Avatar/Avatar.module.css new file mode 100644 index 00000000000..0d65ed72546 --- /dev/null +++ b/packages/react/src/Avatar/Avatar.module.css @@ -0,0 +1,31 @@ +:where(.Avatar) { + display: inline-block; + width: var(--avatarSize-regular); + height: var(--avatarSize-regular); + overflow: hidden; /* Ensure page layout in Firefox should images fail to load */ + line-height: 1; + vertical-align: middle; + border-radius: 50%; + box-shadow: 0 0 0 1px var(--avatar-borderColor); + + &:where([data-square='true']) { + border-radius: var(--borderRadius-medium); + } + + &:where([data-responsive='true']) { + @media screen and (--viewportRange-narrow) { + width: var(--avatarSize-narrow); + height: var(--avatarSize-narrow); + } + + @media screen and (--viewportRange-regular) { + width: var(--avatarSize-regular); + height: var(--avatarSize-regular); + } + + @media screen and (--viewportRange-wide) { + width: var(--avatarSize-wide); + height: var(--avatarSize-wide); + } + } +} diff --git a/packages/react/src/Avatar/Avatar.tsx b/packages/react/src/Avatar/Avatar.tsx index a24622b47c7..bcb07ac13c7 100644 --- a/packages/react/src/Avatar/Avatar.tsx +++ b/packages/react/src/Avatar/Avatar.tsx @@ -1,4 +1,6 @@ +import cx from 'clsx' import React from 'react' +import Box from '../Box' import styled from 'styled-components' import {get} from '../constants' import type {BetterCssProperties, BetterSystemStyleObject, SxProp} from '../sx' @@ -8,6 +10,8 @@ import type {ResponsiveValue} from '../hooks/useResponsiveValue' import {isResponsiveValue} from '../hooks/useResponsiveValue' import {getBreakpointDeclarations} from '../utils/getBreakpointDeclarations' import {defaultSxProp} from '../utils/defaultSxProp' +import classes from './Avatar.module.css' +import {useFeatureFlag} from '../FeatureFlags' export const DEFAULT_AVATAR_SIZE = 20 @@ -41,10 +45,12 @@ const StyledAvatar = styled.img.attrs(props => ({ export type AvatarProps = ComponentProps const Avatar = React.forwardRef(function Avatar( - {alt = '', size = DEFAULT_AVATAR_SIZE, square = false, sx: sxProp = defaultSxProp, ...rest}, + {alt = '', size = DEFAULT_AVATAR_SIZE, square = false, sx: sxProp = defaultSxProp, className, ...rest}, ref, ) { - const avatarSx = isResponsiveValue(size) + const enabled = useFeatureFlag('primer_react_css_modules_team') + const isResponsive = isResponsiveValue(size) + const avatarSx = isResponsive ? merge( getBreakpointDeclarations( size, @@ -54,6 +60,56 @@ const Avatar = React.forwardRef(function Avatar( sxProp as SxProp, ) : merge({'--avatar-size': `${size}px`} as React.CSSProperties, sxProp as SxProp) + + if (enabled) { + const cssSizeVars = {} as React.CSSProperties + + if (isResponsive) { + for (const [key, value] of Object.entries(size)) { + // @ts-ignore - css property + cssSizeVars[`--avatarSize-${key}`] = `${value}px` + } + } else { + // @ts-ignore - css property + cssSizeVars['--avatarSize-regular'] = `${size}px` + } + + if (sxProp !== defaultSxProp) { + return ( + + ) + } + + return ( + {alt} + ) + } + return ( ) From 99de02dabf5366cf0c35ea347d4f23de95cb4575 Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Mon, 26 Aug 2024 15:55:10 -0700 Subject: [PATCH 03/11] Create tame-boats-hide.md --- .changeset/tame-boats-hide.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tame-boats-hide.md diff --git a/.changeset/tame-boats-hide.md b/.changeset/tame-boats-hide.md new file mode 100644 index 00000000000..d13e7497cc4 --- /dev/null +++ b/.changeset/tame-boats-hide.md @@ -0,0 +1,5 @@ +--- +"@primer/react": minor +--- + +Refactor Avatar component to use CSS modules behind feature flag From ae86315bc2337d8e8026cf7f59a5572d4349050a Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Mon, 26 Aug 2024 23:40:43 +0000 Subject: [PATCH 04/11] Add testing in the feature flag --- e2e/components/Avatar.test.ts | 244 ++++++++++++++++++---------------- 1 file changed, 128 insertions(+), 116 deletions(-) diff --git a/e2e/components/Avatar.test.ts b/e2e/components/Avatar.test.ts index dab9f9de809..cbf9922090b 100644 --- a/e2e/components/Avatar.test.ts +++ b/e2e/components/Avatar.test.ts @@ -3,139 +3,151 @@ import {visit} from '../test-helpers/storybook' import {themes} from '../test-helpers/themes' test.describe('Avatar', () => { - test.describe('Default', () => { - for (const theme of themes) { - test.describe(theme, () => { - test('default @vrt', async ({page}) => { - await visit(page, { - id: 'components-avatar--default', - globals: { - colorScheme: theme, - }, - }) + for (const enabled of [true, false]) { + test.describe(`Feature flag enabled: ${enabled}`, () => { + test.describe('Default', () => { + for (const theme of themes) { + test.describe(theme, () => { + test('default @vrt', async ({page}) => { + await visit(page, { + id: 'components-avatar--default', + globals: { + colorScheme: theme, + primer_react_css_modules_team: enabled, + }, + }) - // Default state - expect(await page.screenshot()).toMatchSnapshot(`Avatar.Default.${theme}.png`) - }) + // Default state + expect(await page.screenshot()).toMatchSnapshot(`Avatar.Default.${theme}.png`) + }) - test('axe @aat', async ({page}) => { - await visit(page, { - id: 'components-avatar--default', - globals: { - colorScheme: theme, - }, - }) - await expect(page).toHaveNoViolations({ - rules: { - 'color-contrast': { - enabled: theme !== 'dark_dimmed', - }, - }, + test('axe @aat', async ({page}) => { + await visit(page, { + id: 'components-avatar--default', + globals: { + colorScheme: theme, + primer_react_css_modules_team: enabled, + }, + }) + await expect(page).toHaveNoViolations({ + rules: { + 'color-contrast': { + enabled: theme !== 'dark_dimmed', + }, + }, + }) + }) }) - }) + } }) - } - }) - test.describe('Size', () => { - for (const theme of themes) { - test.describe(theme, () => { - test('default @vrt', async ({page}) => { - await visit(page, { - id: 'components-avatar-features--size', - globals: { - colorScheme: theme, - }, - }) + test.describe('Size', () => { + for (const theme of themes) { + test.describe(theme, () => { + test('default @vrt', async ({page}) => { + await visit(page, { + id: 'components-avatar-features--size', + globals: { + colorScheme: theme, + primer_react_css_modules_team: enabled, + }, + }) - // Default state - expect(await page.screenshot()).toMatchSnapshot(`Avatar.Size.${theme}.png`) - }) + // Default state + expect(await page.screenshot()).toMatchSnapshot(`Avatar.Size.${theme}.png`) + }) - test('axe @aat', async ({page}) => { - await visit(page, { - id: 'components-avatar-features--size', - globals: { - colorScheme: theme, - }, - }) - await expect(page).toHaveNoViolations({ - rules: { - 'color-contrast': { - enabled: theme !== 'dark_dimmed', - }, - }, + test('axe @aat', async ({page}) => { + await visit(page, { + id: 'components-avatar-features--size', + globals: { + colorScheme: theme, + primer_react_css_modules_team: enabled, + }, + }) + await expect(page).toHaveNoViolations({ + rules: { + 'color-contrast': { + enabled: theme !== 'dark_dimmed', + }, + }, + }) + }) }) - }) + } }) - } - }) - test.describe('Size Responsive', () => { - for (const theme of themes) { - test.describe(theme, () => { - test('default @vrt', async ({page}) => { - await visit(page, { - id: 'components-avatar-features--size-responsive', - globals: { - colorScheme: theme, - }, - }) + test.describe('Size Responsive', () => { + for (const theme of themes) { + test.describe(theme, () => { + test('default @vrt', async ({page}) => { + await visit(page, { + id: 'components-avatar-features--size-responsive', + globals: { + colorScheme: theme, + primer_react_css_modules_team: enabled, + }, + }) - // Default state - expect(await page.screenshot()).toMatchSnapshot(`Avatar.Size Responsive.${theme}.png`) - }) + // Default state + expect(await page.screenshot()).toMatchSnapshot(`Avatar.Size Responsive.${theme}.png`) + }) - test('axe @aat', async ({page}) => { - await visit(page, { - id: 'components-avatar-features--size-responsive', - globals: { - colorScheme: theme, - }, - }) - await expect(page).toHaveNoViolations({ - rules: { - 'color-contrast': { - enabled: theme !== 'dark_dimmed', - }, - }, + test('axe @aat', async ({page}) => { + await visit(page, { + id: 'components-avatar-features--size-responsive', + globals: { + colorScheme: theme, + primer_react_css_modules_team: enabled, + }, + }) + await expect(page).toHaveNoViolations({ + rules: { + 'color-contrast': { + enabled: theme !== 'dark_dimmed', + }, + }, + }) + }) }) - }) + } }) - } - }) - test.describe('Square', () => { - for (const theme of themes) { - test.describe(theme, () => { - test('default @vrt', async ({page}) => { - await visit(page, { - id: 'components-avatar-features--square', - globals: { - colorScheme: theme, - }, - }) + test.describe('Square', () => { + for (const theme of themes) { + test.describe(theme, () => { + test('default @vrt', async ({page}) => { + await visit(page, { + id: 'components-avatar-features--square', + globals: { + colorScheme: theme, + primer_react_css_modules_team: enabled, + }, + }) - // Default state - expect(await page.screenshot()).toMatchSnapshot(`Avatar.Square.${theme}.png`) - }) + // Default state + expect(await page.screenshot()).toMatchSnapshot(`Avatar.Square.${theme}.png`) + }) - test('axe @aat', async ({page}) => { - await visit(page, { - id: 'components-avatar-features--square', - globals: { - colorScheme: theme, - }, - }) - await expect(page).toHaveNoViolations({ - rules: { - 'color-contrast': { - enabled: theme !== 'dark_dimmed', - }, - }, + test('axe @aat', async ({page}) => { + await visit(page, { + id: 'components-avatar-features--square', + globals: { + colorScheme: theme, + primer_react_css_modules_team: enabled, + }, + }) + await expect(page).toHaveNoViolations({ + rules: { + 'color-contrast': { + enabled: theme !== 'dark_dimmed', + }, + }, + }) + }) }) - }) + } }) - } - }) + }) + } }) From 8812655f6d23d2155abd34ffe9b6cddd3cc6686f Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Mon, 26 Aug 2024 23:41:23 +0000 Subject: [PATCH 05/11] Convert to require --- packages/react/postcss.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/postcss.config.js b/packages/react/postcss.config.js index d6c4ad59862..29130bbda9f 100644 --- a/packages/react/postcss.config.js +++ b/packages/react/postcss.config.js @@ -1,4 +1,4 @@ -import postcssPresetPrimer from 'postcss-preset-primer' +const postcssPresetPrimer = require('postcss-preset-primer') /** @type {import('postcss-load-config').Config} */ module.exports = postcssPresetPrimer From 847bbb99482f12c29faadb4d873320db42db85e3 Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Tue, 27 Aug 2024 19:08:35 +0000 Subject: [PATCH 06/11] chore: Update postcss.config.js to use postcss-preset-primer --- packages/react/{postcss.config.js => postcss.config.mjs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/react/{postcss.config.js => postcss.config.mjs} (100%) diff --git a/packages/react/postcss.config.js b/packages/react/postcss.config.mjs similarity index 100% rename from packages/react/postcss.config.js rename to packages/react/postcss.config.mjs From 12b1e5658f9514d49fa22494c68aa507481a72e6 Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Tue, 27 Aug 2024 19:15:22 +0000 Subject: [PATCH 07/11] convert to module --- packages/react/postcss.config.mjs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/postcss.config.mjs b/packages/react/postcss.config.mjs index 29130bbda9f..3c7c989934a 100644 --- a/packages/react/postcss.config.mjs +++ b/packages/react/postcss.config.mjs @@ -1,4 +1,4 @@ -const postcssPresetPrimer = require('postcss-preset-primer') +import postcssPresetPrimer from 'postcss-preset-primer' /** @type {import('postcss-load-config').Config} */ -module.exports = postcssPresetPrimer +export default postcssPresetPrimer From 6e5bdfd1981336a093ec6f9171bf29b322684081 Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Thu, 5 Sep 2024 17:57:12 +0000 Subject: [PATCH 08/11] Pass along className to StyledAvatar --- packages/react/src/Avatar/Avatar.tsx | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/react/src/Avatar/Avatar.tsx b/packages/react/src/Avatar/Avatar.tsx index bcb07ac13c7..ab4a36642c2 100644 --- a/packages/react/src/Avatar/Avatar.tsx +++ b/packages/react/src/Avatar/Avatar.tsx @@ -111,7 +111,16 @@ const Avatar = React.forwardRef(function Avatar( } return ( - + ) }) From f15d5b29d7b32c306bd2851bb86da9424020e68a Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Thu, 5 Sep 2024 18:18:58 +0000 Subject: [PATCH 09/11] fix for lint --- packages/react/src/Avatar/Avatar.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react/src/Avatar/Avatar.tsx b/packages/react/src/Avatar/Avatar.tsx index ab4a36642c2..54593373cfb 100644 --- a/packages/react/src/Avatar/Avatar.tsx +++ b/packages/react/src/Avatar/Avatar.tsx @@ -1,4 +1,4 @@ -import cx from 'clsx' +import {clsx} from 'clsx' import React from 'react' import Box from '../Box' import styled from 'styled-components' @@ -79,7 +79,7 @@ const Avatar = React.forwardRef(function Avatar( (function Avatar( return ( {alt} Date: Wed, 11 Sep 2024 17:23:19 +0000 Subject: [PATCH 10/11] Render boolean props with '' --- packages/react/src/Avatar/Avatar.module.css | 4 ++-- packages/react/src/Avatar/Avatar.tsx | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react/src/Avatar/Avatar.module.css b/packages/react/src/Avatar/Avatar.module.css index 0d65ed72546..db820ccc99f 100644 --- a/packages/react/src/Avatar/Avatar.module.css +++ b/packages/react/src/Avatar/Avatar.module.css @@ -8,11 +8,11 @@ border-radius: 50%; box-shadow: 0 0 0 1px var(--avatar-borderColor); - &:where([data-square='true']) { + &:where([data-square]) { border-radius: var(--borderRadius-medium); } - &:where([data-responsive='true']) { + &:where([data-responsive]) { @media screen and (--viewportRange-narrow) { width: var(--avatarSize-narrow); height: var(--avatarSize-narrow); diff --git a/packages/react/src/Avatar/Avatar.tsx b/packages/react/src/Avatar/Avatar.tsx index 54593373cfb..441c97e2411 100644 --- a/packages/react/src/Avatar/Avatar.tsx +++ b/packages/react/src/Avatar/Avatar.tsx @@ -82,8 +82,8 @@ const Avatar = React.forwardRef(function Avatar( className={clsx(className, classes.Avatar)} ref={ref} alt={alt} - data-responsive={isResponsive ? 'true' : undefined} - data-square={square ? 'true' : undefined} + data-responsive={isResponsive ? '' : undefined} + data-square={square ? '' : undefined} width={isResponsive ? undefined : size} height={isResponsive ? undefined : size} // @ts-ignore - it's not allowing CSS properties here From 46da58190e27ecbcd143e8f8bbe6d9570b1ad099 Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Wed, 11 Sep 2024 17:30:06 +0000 Subject: [PATCH 11/11] Adjust typescript stuff --- packages/react/src/Avatar/Avatar.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/react/src/Avatar/Avatar.tsx b/packages/react/src/Avatar/Avatar.tsx index 441c97e2411..2f4cb8d0a5d 100644 --- a/packages/react/src/Avatar/Avatar.tsx +++ b/packages/react/src/Avatar/Avatar.tsx @@ -62,15 +62,13 @@ const Avatar = React.forwardRef(function Avatar( : merge({'--avatar-size': `${size}px`} as React.CSSProperties, sxProp as SxProp) if (enabled) { - const cssSizeVars = {} as React.CSSProperties + const cssSizeVars = {} as Record if (isResponsive) { for (const [key, value] of Object.entries(size)) { - // @ts-ignore - css property cssSizeVars[`--avatarSize-${key}`] = `${value}px` } } else { - // @ts-ignore - css property cssSizeVars['--avatarSize-regular'] = `${size}px` } @@ -87,7 +85,7 @@ const Avatar = React.forwardRef(function Avatar( width={isResponsive ? undefined : size} height={isResponsive ? undefined : size} // @ts-ignore - it's not allowing CSS properties here - style={cssSizeVars} + style={cssSizeVars as React.CSSProperties} sx={sxProp} {...rest} /> @@ -104,7 +102,7 @@ const Avatar = React.forwardRef(function Avatar( width={isResponsive ? undefined : size} height={isResponsive ? undefined : size} // @ts-ignore - it's not allowing CSS properties here - style={cssSizeVars} + style={cssSizeVars as React.CSSProperties} {...rest} /> )