chore(BaseStyles): Remove the CSS modules feature flag from the BaseStyles component #5981

jonrohan · Apr 28, 2025

Closes https://github.com/github/primer/issues/4358

Changelog

New

Changed

Removed

Remove the CSS modules feature flag from the BaseStyles component

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

… and enhance storybook tests

changeset-bot · Apr 28, 2025

🦋 Changeset detected

Latest commit: 70af75c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · Apr 28, 2025

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

github-actions · Apr 28, 2025

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	98.95 KB (-0.14% 🔽)
packages/react/dist/browser.umd.js	99.21 KB (-0.18% 🔽)

primer-integration · Apr 28, 2025

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/375222

primer-integration · Apr 28, 2025

🟢 golden-jobs completed with status success.

Copilot

Pull Request Overview

This PR removes the CSS modules feature flag from the BaseStyles component and simplifies the component rendering logic. Key changes include skipping certain tests related to styling and system props, removing the feature flag logic and associated styled‐components code from BaseStyles.tsx, and updating stories and e2e tests accordingly.

Reviewed Changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/react/src/tests/BaseStyles.test.tsx	Temporarily skipping tests related to styling and system props
packages/react/src/BaseStyles.tsx	Removing CSS modules feature flag and associated styled‐components logic; introducing BoxWithFallback
packages/react/src/BaseStyles.dev.stories.tsx	Adding stories to showcase new BaseStyles behavior using sx, system, and style props
e2e/components/BaseStyles.test.ts	Expanding e2e test coverage to include the new story variants
.changeset/cuddly-facts-speak.md	Documenting the removal of the feature flag in the changeset

Files not reviewed (2)

packages/react/src/tests/snapshots/AnchoredOverlay.test.tsx.snap: Language not supported
packages/react/src/tests/snapshots/BaseStyles.test.tsx.snap: Language not supported

Comments suppressed due to low confidence (2)

packages/react/src/BaseStyles.tsx:7

The CSS modules import and associated CSS classes are no longer used; please consider removing this import to avoid unused code.

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

packages/react/src/BaseStyles.tsx:1

Since the styled-components logic has been removed along with the feature flag, this import can also be removed to clean up the file.

import styled, {createGlobalStyle} from 'styled-components'

Copilot · Apr 28, 2025

packages/react/src/__tests__/BaseStyles.test.tsx

@@ -20,7 +20,7 @@ describe('BaseStyles', () => {
    expect(container).toMatchSnapshot()
  })

-  it('respects styling props', () => {
+  it.skip('respects styling props', () => {


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.

Seems like a good idea to add this

github-actions · Apr 28, 2025

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

francinelucca

some questionsss

results.json

francinelucca · Apr 29, 2025

packages/react/src/__tests__/BaseStyles.test.tsx

@@ -20,7 +20,7 @@ describe('BaseStyles', () => {
    expect(container).toMatchSnapshot()
  })

-  it('respects styling props', () => {
+  it.skip('respects styling props', () => {


why do we need to skip these now? 🤔

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.

francinelucca · Apr 29, 2025

packages/react/src/BaseStyles.tsx

-    if (includesSystemProps(props)) {
-      const systemProps = getTypographyAndCommonProps(props)


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

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

react/packages/react/src/internal/components/BoxWithFallback.tsx

Line 11 in 36f1007

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

…ressBar component (#5983)

…component (#5982)

github-actions · Apr 29, 2025

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

hectahertz · Apr 30, 2025

packages/react/src/__tests__/BaseStyles.test.tsx

@@ -20,7 +20,7 @@ describe('BaseStyles', () => {
    expect(container).toMatchSnapshot()
  })

-  it('respects styling props', () => {
+  it.skip('respects styling props', () => {


Seems like a good idea to add this

…tyles component (#5981) Co-authored-by: jonrohan <54012+jonrohan@users.noreply.github.com> Co-authored-by: Josh Black <joshblack@github.com>

chore: update BaseStyles component to remove CSS modules feature flag…

dd2a649

… and enhance storybook tests

github-actions bot added staff Author is a staff member integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Apr 28, 2025

Skip toHaveStyle tests

1451689

github-actions bot requested a deployment to storybook-preview-5981 April 28, 2025 20:40 Abandoned

jonrohan deployed to github-pages April 28, 2025 20:52 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-5981 April 28, 2025 20:53 Inactive

jonrohan added the update snapshots label Apr 28, 2025

github-actions bot added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Apr 28, 2025

jonrohan marked this pull request as ready for review April 28, 2025 21:02

Copilot AI review requested due to automatic review settings April 28, 2025 21:02

jonrohan requested a review from a team as a code owner April 28, 2025 21:02

jonrohan requested review from hectahertz and francinelucca April 28, 2025 21:02

Copilot AI reviewed Apr 28, 2025

View reviewed changes

test(vrt): update snapshots

23277e5

primer bot requested a review from a team as a code owner April 28, 2025 21:12

primer bot requested a review from emilybrick April 28, 2025 21:12

github-actions bot added integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm and removed update snapshots labels Apr 28, 2025

jonrohan removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Apr 28, 2025

primer bot deployed to github-pages April 28, 2025 21:27 View deployment

github-actions bot temporarily deployed to storybook-preview-5981 April 28, 2025 21:27 Inactive

jonrohan enabled auto-merge April 28, 2025 22:09

francinelucca reviewed Apr 29, 2025

View reviewed changes

jonrohan and others added 5 commits April 29, 2025 10:32

Delete results.json

1bbaa1f

feat: add automatic transform for jsx runtime (#5973)

0f002a6

chore(ProgressBar): Remove the CSS modules feature flag from the Prog…

d287517

…ressBar component (#5983)

chore(Overlay): Remove the CSS modules feature flag from the Overlay …

0b9f77a

…component (#5982)

Merge branch 'main' into css_modules_remove_flag/basestyles

70af75c

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Apr 29, 2025

jonrohan deployed to github-pages April 29, 2025 17:58 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-5981 April 29, 2025 17:58 View deployment

hectahertz approved these changes Apr 30, 2025

View reviewed changes

jonrohan added this pull request to the merge queue Apr 30, 2025

Merged via the queue into main with commit b03f78f Apr 30, 2025
45 checks passed

jonrohan deleted the css_modules_remove_flag/basestyles branch April 30, 2025 14:03

primer bot mentioned this pull request Apr 29, 2025

Release tracking #5966

Merged

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

		if (includesSystemProps(props)) {
		const systemProps = getTypographyAndCommonProps(props)

Search code, repositories, users, issues, pull requests...

chore(BaseStyles): Remove the CSS modules feature flag from the BaseStyles component #5981

chore(BaseStyles): Remove the CSS modules feature flag from the BaseStyles component #5981

Uh oh!

Conversation

jonrohan commented Apr 28, 2025

Changelog

New

Changed

Removed

Rollout strategy

Merge checklist

Uh oh!

changeset-bot bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Apr 28, 2025

Uh oh!

github-actions bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

primer-integration bot commented Apr 28, 2025

Uh oh!

primer-integration bot commented Apr 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

hectahertz Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 28, 2025

Uh oh!

francinelucca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

francinelucca Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

jonrohan Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

francinelucca Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

jonrohan Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 29, 2025

Uh oh!

hectahertz Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

changeset-bot bot commented Apr 28, 2025 •

edited

Loading

github-actions bot commented Apr 28, 2025 •

edited

Loading