From e15d49a090587e863eb2b85cb275001a754b1fab Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Fri, 13 Dec 2024 22:52:24 +0000 Subject: [PATCH 1/6] Fix Typography and Common props for BaseStyles behind feature flag --- packages/react/src/BaseStyles.dev.stories.tsx | 3 +- packages/react/src/BaseStyles.tsx | 111 ++++++++++++++---- .../react/src/__tests__/BaseStyles.test.tsx | 19 ++- .../__snapshots__/BaseStyles.test.tsx.snap | 8 +- .../react/src/utils/includeSystemProps.ts | 17 +++ 5 files changed, 126 insertions(+), 32 deletions(-) create mode 100644 packages/react/src/utils/includeSystemProps.ts diff --git a/packages/react/src/BaseStyles.dev.stories.tsx b/packages/react/src/BaseStyles.dev.stories.tsx index d313f089009..c778d9f0259 100644 --- a/packages/react/src/BaseStyles.dev.stories.tsx +++ b/packages/react/src/BaseStyles.dev.stories.tsx @@ -1,6 +1,5 @@ import {BaseStyles} from '.' import type {Meta} from '@storybook/react' -import React from 'react' import type {ComponentProps} from './utils/types' export default { @@ -8,4 +7,4 @@ export default { component: BaseStyles, } as Meta> -export const Default = () => Hello +export const Default = () => 'Hello' diff --git a/packages/react/src/BaseStyles.tsx b/packages/react/src/BaseStyles.tsx index 2a5ffb24347..1ab10440b37 100644 --- a/packages/react/src/BaseStyles.tsx +++ b/packages/react/src/BaseStyles.tsx @@ -1,12 +1,14 @@ -import React from 'react' +import React, {type CSSProperties, type PropsWithChildren} from 'react' import {clsx} from 'clsx' import styled, {createGlobalStyle} from 'styled-components' -import type {ComponentProps} from './utils/types' import type {SystemCommonProps, SystemTypographyProps} from './constants' import {COMMON, TYPOGRAPHY} from './constants' import {useTheme} from './ThemeProvider' import {useFeatureFlag} from './FeatureFlags' -import {toggleStyledComponent} from './internal/utils/toggleStyledComponent' +import Box from './Box' +import type {SxProp} from './sx' +import {includesSystemProps} from './utils/includeSystemProps' + import classes from './BaseStyles.module.css' // load polyfill for :focus-visible @@ -35,45 +37,106 @@ const GlobalStyle = createGlobalStyle<{colorScheme?: 'light' | 'dark'}>` } ` -const Base = toggleStyledComponent( - CSS_MODULES_FEATURE_FLAG, - 'div', - styled.div` - ${TYPOGRAPHY}; - ${COMMON}; - `, -) +const StyledDiv = styled.div` + ${TYPOGRAPHY}; + ${COMMON}; +` -export type BaseStylesProps = ComponentProps +export type BaseStylesProps = PropsWithChildren & { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + as?: React.ComponentType | keyof JSX.IntrinsicElements + className?: string + style?: CSSProperties + color?: string // Fixes `color` ts error +} & SystemTypographyProps & + SystemCommonProps & + SxProp function BaseStyles(props: BaseStylesProps) { - const {children, color = 'fg.default', fontFamily = 'normal', lineHeight = 'default', className, ...rest} = props + const { + children, + color = 'var(--fgColor-default)', + fontFamily = 'normal', + lineHeight = 'default', + className, + as: Component = 'div', + ...rest + } = props const {colorScheme, dayScheme, nightScheme} = useTheme() const enabled = useFeatureFlag(CSS_MODULES_FEATURE_FLAG) - const stylingProps = enabled ? {className: clsx(classes.BaseStyles, className)} : {className} + if (enabled) { + const newClassName = clsx(classes.BaseStyles, className) + + // If props includes TYPOGRAPHY or COMMON props, pass them to the Box component + if (includesSystemProps(props)) { + return ( + // @ts-ignore shh + + {children} + + ) + } + + return ( + + {children} + + ) + } - /** - * We need to map valid primer/react color modes onto valid color modes for primer/primitives - * valid color modes for primer/primitives: auto | light | dark - * valid color modes for primer/primer: auto | day | night | light | dark - */ return ( - - {!enabled && } + {children} - + ) } diff --git a/packages/react/src/__tests__/BaseStyles.test.tsx b/packages/react/src/__tests__/BaseStyles.test.tsx index 8895f46d3d8..17c0ce31923 100644 --- a/packages/react/src/__tests__/BaseStyles.test.tsx +++ b/packages/react/src/__tests__/BaseStyles.test.tsx @@ -16,7 +16,7 @@ describe('BaseStyles', () => { }) it('has default styles', () => { - const {container} = render() + const {container} = render(Hello) expect(container).toMatchSnapshot() }) @@ -27,10 +27,23 @@ describe('BaseStyles', () => { lineHeight: '3.5', } - const {container} = render() + const {container} = render(Hello) expect(container.children[0]).toHaveStyle({color: '#f00', 'font-family': 'Arial', 'line-height': '3.5'}) }) + it('respects system props', () => { + const {container} = render( + + Hello + , + ) + + expect(container.children[0]).toHaveStyle({ + display: 'contents', + 'white-space': 'pre-wrap', + }) + }) + it('accepts className and style props', () => { const styles = { style: {margin: '10px'}, @@ -38,7 +51,7 @@ describe('BaseStyles', () => { sx: {}, } - const {container} = render() + const {container} = render(Hello) expect(container.children[0]).toHaveClass('test-classname') expect(container.children[0]).toHaveStyle({margin: '10px'}) }) diff --git a/packages/react/src/__tests__/__snapshots__/BaseStyles.test.tsx.snap b/packages/react/src/__tests__/__snapshots__/BaseStyles.test.tsx.snap index 18c719e6bff..4dab403c115 100644 --- a/packages/react/src/__tests__/__snapshots__/BaseStyles.test.tsx.snap +++ b/packages/react/src/__tests__/__snapshots__/BaseStyles.test.tsx.snap @@ -4,16 +4,18 @@ exports[`BaseStyles has default styles 1`] = ` .c0 { font-family: normal; line-height: default; - color: fg.default; + color: var(--fgColor-default); }
+ > + Hello +
`; diff --git a/packages/react/src/utils/includeSystemProps.ts b/packages/react/src/utils/includeSystemProps.ts new file mode 100644 index 00000000000..1d6826b7a59 --- /dev/null +++ b/packages/react/src/utils/includeSystemProps.ts @@ -0,0 +1,17 @@ +import {COMMON, TYPOGRAPHY} from '../constants' +import type {SxProp} from '../sx' + +const COMMON_PROP_NAMES = new Set(Object.keys(COMMON)) +const TYPOGRAPHY_PROP_NAMES = new Set(Object.keys(TYPOGRAPHY)) + +const includesSystemProps = (props: SxProp) => { + if (props.sx) { + return true + } + + return Object.keys(props).some(prop => { + return TYPOGRAPHY_PROP_NAMES.has(prop) || COMMON_PROP_NAMES.has(prop) + }) +} + +export {includesSystemProps} From a990f912e7af33189c91985c0de0a0b95e5688e1 Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Fri, 13 Dec 2024 22:53:45 +0000 Subject: [PATCH 2/6] Update Text to use util includesSystemProps --- packages/react/src/Text/Text.tsx | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/packages/react/src/Text/Text.tsx b/packages/react/src/Text/Text.tsx index 9bad8d5d923..a6cd88e276f 100644 --- a/packages/react/src/Text/Text.tsx +++ b/packages/react/src/Text/Text.tsx @@ -8,8 +8,9 @@ import sx from '../sx' import {useFeatureFlag} from '../FeatureFlags' import Box from '../Box' import {useRefObjectAsForwardedRef} from '../hooks' -import classes from './Text.module.css' import type {ComponentProps} from '../utils/types' +import {includesSystemProps} from '../utils/includeSystemProps' +import classes from './Text.module.css' type StyledTextProps = { // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -58,19 +59,6 @@ const StyledText = styled.span` ${sx}; ` -const COMMON_PROP_NAMES = new Set(Object.keys(COMMON)) -const TYPOGRAPHY_PROP_NAMES = new Set(Object.keys(TYPOGRAPHY)) - -const includesSystemProps = (props: StyledTextProps) => { - if (props.sx) { - return true - } - - return Object.keys(props).some(prop => { - return TYPOGRAPHY_PROP_NAMES.has(prop) || COMMON_PROP_NAMES.has(prop) - }) -} - const Text = forwardRef(({as: Component = 'span', className, size, weight, ...props}, forwardedRef) => { const enabled = useFeatureFlag('primer_react_css_modules_ga') From ec5f0c69b54a211fcdaf402bb0dd97065a25032d Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Sat, 14 Dec 2024 00:29:59 +0000 Subject: [PATCH 3/6] Fix whiteSpace prop for BaseStyles --- packages/react/src/BaseStyles.tsx | 5 +++-- .../react/src/__tests__/BaseStyles.test.tsx | 3 ++- packages/react/src/utils/includeSystemProps.ts | 18 ++++++++++++++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/packages/react/src/BaseStyles.tsx b/packages/react/src/BaseStyles.tsx index 1ab10440b37..87af4e846a1 100644 --- a/packages/react/src/BaseStyles.tsx +++ b/packages/react/src/BaseStyles.tsx @@ -7,7 +7,7 @@ import {useTheme} from './ThemeProvider' import {useFeatureFlag} from './FeatureFlags' import Box from './Box' import type {SxProp} from './sx' -import {includesSystemProps} from './utils/includeSystemProps' +import {includesSystemProps, getTypographyAndCommonProps} from './utils/includeSystemProps' import classes from './BaseStyles.module.css' @@ -47,7 +47,6 @@ export type BaseStylesProps = PropsWithChildren & { as?: React.ComponentType | keyof JSX.IntrinsicElements className?: string style?: CSSProperties - color?: string // Fixes `color` ts error } & SystemTypographyProps & SystemCommonProps & SxProp @@ -71,6 +70,7 @@ function BaseStyles(props: BaseStylesProps) { // If props includes TYPOGRAPHY or COMMON props, pass them to the Box component if (includesSystemProps(props)) { + const systemProps = getTypographyAndCommonProps(props) return ( // @ts-ignore shh {children} diff --git a/packages/react/src/__tests__/BaseStyles.test.tsx b/packages/react/src/__tests__/BaseStyles.test.tsx index 17c0ce31923..c6d1270b348 100644 --- a/packages/react/src/__tests__/BaseStyles.test.tsx +++ b/packages/react/src/__tests__/BaseStyles.test.tsx @@ -33,7 +33,7 @@ describe('BaseStyles', () => { it('respects system props', () => { const {container} = render( - + Hello , ) @@ -41,6 +41,7 @@ describe('BaseStyles', () => { expect(container.children[0]).toHaveStyle({ display: 'contents', 'white-space': 'pre-wrap', + 'margin-right': '8px', }) }) diff --git a/packages/react/src/utils/includeSystemProps.ts b/packages/react/src/utils/includeSystemProps.ts index 1d6826b7a59..c958e79144e 100644 --- a/packages/react/src/utils/includeSystemProps.ts +++ b/packages/react/src/utils/includeSystemProps.ts @@ -1,4 +1,4 @@ -import {COMMON, TYPOGRAPHY} from '../constants' +import {COMMON, TYPOGRAPHY, type SystemCommonProps, type SystemTypographyProps} from '../constants' import type {SxProp} from '../sx' const COMMON_PROP_NAMES = new Set(Object.keys(COMMON)) @@ -14,4 +14,18 @@ const includesSystemProps = (props: SxProp) => { }) } -export {includesSystemProps} +type TypographyAndCommonProp = SystemTypographyProps & SystemCommonProps + +const getTypographyAndCommonProps = (props: TypographyAndCommonProp): TypographyAndCommonProp => { + let typographyAndCommonProps = {} as TypographyAndCommonProp + for (const prop of Object.keys(props)) { + if (TYPOGRAPHY_PROP_NAMES.has(prop) || COMMON_PROP_NAMES.has(prop)) { + const p = prop as keyof TypographyAndCommonProp + typographyAndCommonProps = {...typographyAndCommonProps, [p]: props[p]} + } + } + + return typographyAndCommonProps +} + +export {includesSystemProps, getTypographyAndCommonProps} From b4fe15c073694ac13e3441a01f6a086dfb4a47e6 Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Sat, 14 Dec 2024 00:31:49 +0000 Subject: [PATCH 4/6] Add changeset --- .changeset/polite-books-sneeze.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/polite-books-sneeze.md diff --git a/.changeset/polite-books-sneeze.md b/.changeset/polite-books-sneeze.md new file mode 100644 index 00000000000..e56d828b817 --- /dev/null +++ b/.changeset/polite-books-sneeze.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Fix Typography and Common props for BaseStyles when CSS modules feature flag is enabled From 1d6f3768b15886c68ea63c6f3d5011c208531ae7 Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Sat, 14 Dec 2024 01:04:35 +0000 Subject: [PATCH 5/6] Fix color ts error --- packages/react/src/BaseStyles.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react/src/BaseStyles.tsx b/packages/react/src/BaseStyles.tsx index 87af4e846a1..f44cc58564c 100644 --- a/packages/react/src/BaseStyles.tsx +++ b/packages/react/src/BaseStyles.tsx @@ -47,6 +47,7 @@ export type BaseStylesProps = PropsWithChildren & { as?: React.ComponentType | keyof JSX.IntrinsicElements className?: string style?: CSSProperties + color?: string // Fixes `color` ts-error } & SystemTypographyProps & SystemCommonProps & SxProp From 5a702344c98bf46e213ef29236988f735391bf55 Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Sat, 14 Dec 2024 01:11:11 +0000 Subject: [PATCH 6/6] Updated color variable in AnchoredOverlay --- .../src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap b/packages/react/src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap index b09b34fe96f..1c8391c7328 100644 --- a/packages/react/src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap +++ b/packages/react/src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap @@ -4,7 +4,7 @@ exports[`AnchoredOverlay should render consistently when open 1`] = ` .c0 { font-family: -apple-system,BlinkMacSystemFont,"Segoe UI","Noto Sans",Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji"; line-height: 1.5; - color: var(--fgColor-default,var(--color-fg-default,#1F2328)); + color: var(--fgColor-default); } .c1 { @@ -38,7 +38,7 @@ exports[`AnchoredOverlay should render consistently when open 1`] = `