From 373ce95042a4e2244a220378dccfc03fa001e7cf Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Wed, 11 Sep 2024 10:43:48 -0700 Subject: [PATCH] refactor(Avatar): Refactor Avatar component to use CSS modules behind feature flag (#4885) * Import postcsspresetprimer to storybook config * Converting to CSS module * Create tame-boats-hide.md * Add testing in the feature flag * Convert to require * chore: Update postcss.config.js to use postcss-preset-primer * convert to module * Pass along className to StyledAvatar * fix for lint * Render boolean props with '' * Adjust typescript stuff --- .changeset/tame-boats-hide.md | 5 + e2e/components/Avatar.test.ts | 244 +++++++++--------- .../src/Avatar/Avatar.features.stories.tsx | 10 + packages/react/src/Avatar/Avatar.module.css | 31 +++ packages/react/src/Avatar/Avatar.tsx | 69 ++++- 5 files changed, 240 insertions(+), 119 deletions(-) create mode 100644 .changeset/tame-boats-hide.md create mode 100644 packages/react/src/Avatar/Avatar.module.css 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 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', + }, + }, + }) + }) }) - }) + } }) - } - }) + }) + } }) 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..db820ccc99f --- /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]) { + border-radius: var(--borderRadius-medium); + } + + &:where([data-responsive]) { + @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..2f4cb8d0a5d 100644 --- a/packages/react/src/Avatar/Avatar.tsx +++ b/packages/react/src/Avatar/Avatar.tsx @@ -1,4 +1,6 @@ +import {clsx} 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,8 +60,65 @@ 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 Record + + if (isResponsive) { + for (const [key, value] of Object.entries(size)) { + cssSizeVars[`--avatarSize-${key}`] = `${value}px` + } + } else { + cssSizeVars['--avatarSize-regular'] = `${size}px` + } + + if (sxProp !== defaultSxProp) { + return ( + + ) + } + + return ( + {alt} + ) + } + return ( - + ) })