Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/cuddly-facts-speak.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Remove the CSS modules feature flag from the BaseStyles component
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions e2e/components/BaseStyles.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,18 @@ const stories = [
id: 'behaviors-basestyles-dev--default',
title: 'Dev Default',
},
{
id: 'behaviors-basestyles-dev--with-style-props',
title: 'Dev With Style Props',
},
{
id: 'behaviors-basestyles-dev--with-sx-props',
title: 'Dev With Sx Props',
},
{
id: 'behaviors-basestyles-dev--with-system-props',
title: 'Dev With System Props',
},
] as const

test.describe('BaseStyles', () => {
Expand Down
33 changes: 33 additions & 0 deletions packages/react/src/BaseStyles.dev.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,43 @@
import {BaseStyles} from '.'
import type {Meta} from '@storybook/react'
import type {ComponentProps} from './utils/types'
import React from 'react'

export default {
title: 'Behaviors/BaseStyles/Dev',
component: BaseStyles,
} as Meta<ComponentProps<typeof BaseStyles>>

export const Default = () => 'Hello'

export const WithSxProps = () => (
<BaseStyles
sx={{
color: 'red',
backgroundColor: 'blue',
fontFamily: 'Arial',
lineHeight: '1.5',
}}
>
Hello
</BaseStyles>
)

export const WithSystemProps = () => (
<BaseStyles color="red" backgroundColor="blue" fontFamily="Arial" fontSize="14px" lineHeight="1.5" display="flex">
Hello
</BaseStyles>
)

export const WithStyleProps = () => (
<BaseStyles
style={{
color: 'red',
backgroundColor: 'blue',
fontFamily: 'Arial',
lineHeight: '1.5',
}}
>
Hello
</BaseStyles>
)
127 changes: 24 additions & 103 deletions packages/react/src/BaseStyles.tsx
Original file line number Diff line number Diff line change
@@ -1,46 +1,13 @@
import React, {type CSSProperties, type PropsWithChildren} from 'react'
import {clsx} from 'clsx'
import styled, {createGlobalStyle} from 'styled-components'
import type {SystemCommonProps, SystemTypographyProps} from './constants'
import {COMMON, TYPOGRAPHY} from './constants'
import {useTheme} from './ThemeProvider'
import {useFeatureFlag} from './FeatureFlags'
import Box from './Box'
import type {SxProp} from './sx'
import {includesSystemProps, getTypographyAndCommonProps} from './utils/includeSystemProps'

import classes from './BaseStyles.module.css'

// load polyfill for :focus-visible
import 'focus-visible'

const CSS_MODULES_FEATURE_FLAG = 'primer_react_css_modules_ga'

const GlobalStyle = createGlobalStyle<{colorScheme?: 'light' | 'dark'}>`
* { box-sizing: border-box; }
body { margin: 0; }
table { border-collapse: collapse; }
input { color-scheme: ${props => props.colorScheme}; }
[role="button"]:focus:not(:focus-visible):not(.focus-visible),
[role="tabpanel"][tabindex="0"]:focus:not(:focus-visible):not(.focus-visible),
button:focus:not(:focus-visible):not(.focus-visible),
summary:focus:not(:focus-visible):not(.focus-visible),
a:focus:not(:focus-visible):not(.focus-visible) {
outline: none;
box-shadow: none;
}
[tabindex="0"]:focus:not(:focus-visible):not(.focus-visible),
details-dialog:focus:not(:focus-visible):not(.focus-visible) {
outline: none;
}
`

const StyledDiv = styled.div<SystemTypographyProps & SystemCommonProps>`
${TYPOGRAPHY};
${COMMON};
`
import {BoxWithFallback} from './internal/components/BoxWithFallback'

export type BaseStylesProps = PropsWithChildren & {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand All @@ -52,77 +19,29 @@ export type BaseStylesProps = PropsWithChildren & {
SystemCommonProps &
SxProp

function BaseStyles(props: BaseStylesProps) {
const {children, color, fontFamily, lineHeight, className, as: Component = 'div', style, ...rest} = props
function BaseStyles({
children,
color,
fontFamily,
lineHeight,
className,
as: Component = 'div',
style,
...rest
}: BaseStylesProps) {
const {colorMode, colorScheme, dayScheme, nightScheme} = useTheme()
const enabled = useFeatureFlag(CSS_MODULES_FEATURE_FLAG)

if (enabled) {
const newClassName = clsx(classes.BaseStyles, className)
const baseStyles = {
['--BaseStyles-fgColor']: color,
['--BaseStyles-fontFamily']: fontFamily,
['--BaseStyles-lineHeight']: lineHeight,
}

// If props includes TYPOGRAPHY or COMMON props, pass them to the Box component
if (includesSystemProps(props)) {
const systemProps = getTypographyAndCommonProps(props)
Comment on lines -69 to -70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why this was needed before but not anymore? I'm confused since this seems to be the old-enabled version 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to check if system props were coming in, but we've abstracted that same functionality to the BoxWithFallback component so no need to double check now

if (sx !== defaultSxProp || includesSystemProps(rest)) {

return (
// @ts-ignore shh
<Box
as={Component}
className={newClassName}
data-portal-root
/**
* 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
*/
data-color-mode={colorMode === 'auto' ? 'auto' : colorScheme?.includes('dark') ? 'dark' : 'light'}
data-light-theme={dayScheme}
data-dark-theme={nightScheme}
style={{
...systemProps,
...baseStyles,
...style,
}}
{...rest}
>
{children}
</Box>
)
}

return (
<Component
className={newClassName}
data-portal-root
/**
* 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
*/
data-color-mode={colorMode === 'auto' ? 'auto' : colorScheme?.includes('dark') ? 'dark' : 'light'}
data-light-theme={dayScheme}
data-dark-theme={nightScheme}
style={{
...baseStyles,
...style,
}}
{...rest}
>
{children}
</Component>
)
const newClassName = clsx(classes.BaseStyles, className)
const baseStyles = {
['--BaseStyles-fgColor']: color,
['--BaseStyles-fontFamily']: fontFamily,
['--BaseStyles-lineHeight']: lineHeight,
}

return (
<StyledDiv
className={className}
color={color ?? 'var(--fgColor-default)'}
fontFamily={fontFamily ?? 'normal'}
lineHeight={lineHeight ?? 'default'}
<BoxWithFallback
as={Component}
className={newClassName}
data-portal-root
/**
* We need to map valid primer/react color modes onto valid color modes for primer/primitives
Expand All @@ -132,12 +51,14 @@ function BaseStyles(props: BaseStylesProps) {
data-color-mode={colorMode === 'auto' ? 'auto' : colorScheme?.includes('dark') ? 'dark' : 'light'}
data-light-theme={dayScheme}
data-dark-theme={nightScheme}
style={style}
style={{
...baseStyles,
...style,
}}
{...rest}
>
<GlobalStyle colorScheme={colorScheme?.includes('dark') ? 'dark' : 'light'} />
{children}
</StyledDiv>
</BoxWithFallback>
)
}

Expand Down
6 changes: 3 additions & 3 deletions packages/react/src/__tests__/BaseStyles.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('BaseStyles', () => {
expect(container).toMatchSnapshot()
})

it('respects styling props', () => {
it.skip('respects styling props', () => {
Copy link

Copilot AI Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an inline comment to explain why these tests are being skipped, so future maintainers understand the temporary nature of this change.

Suggested change
it.skip('respects styling props', () => {
it.skip('respects styling props', () => {
// Skipping this test temporarily due to incomplete implementation of styling props handling.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea to add this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to skip these now? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .toHaveStyle check doesn't work anymore because of the CSS modules not being included in the test suite. I think eventually we'll figure out something and un-skip all these, but I'm not sure what we can do here in the meantime so I've been skipping them.

const styles = {
color: '#f00',
fontFamily: 'Arial',
Expand All @@ -31,7 +31,7 @@ describe('BaseStyles', () => {
expect(container.children[0]).toHaveStyle({color: '#f00', 'font-family': 'Arial', 'line-height': '3.5'})
})

it('respects system props', () => {
it.skip('respects system props', () => {
const {container} = render(
<BaseStyles display="contents" whiteSpace="pre-wrap" mr="2">
Hello
Expand All @@ -45,7 +45,7 @@ describe('BaseStyles', () => {
})
})

it('accepts className and style props', () => {
it.skip('accepts className and style props', () => {
const styles = {
style: {margin: '10px'},
className: 'test-classname',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,13 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

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);
}

<div>
<div
class="c0"
color="var(--fgColor-default)"
class="BaseStyles"
data-color-mode="light"
data-dark-theme="dark"
data-light-theme="light"
data-portal-root="true"
font-family="normal"
>
<button
aria-describedby=":rj:-loading-announcement"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`BaseStyles has default styles 1`] = `
.c0 {
font-family: normal;
line-height: default;
color: var(--fgColor-default);
}

<div>
<div
class="c0"
color="var(--fgColor-default)"
class="BaseStyles"
data-color-mode="light"
data-portal-root="true"
font-family="normal"
>
Hello
</div>
Expand Down
Loading