Skip to content

migrate TreeView to CSS modules - #5267

Merged
jonrohan merged 19 commits into
mainfrom
ktravers/rm-sx-from-tree-view
Nov 14, 2024
Merged

migrate TreeView to CSS modules#5267
jonrohan merged 19 commits into
mainfrom
ktravers/rm-sx-from-tree-view

Conversation

@ktravers

@ktraversktravers commented Nov 12, 2024•
edited by jonrohan
Loading

Copy link
Copy Markdown
Contributor

closeshttps://github.com/github/primer/issues/4407

This PR migrates the TreeView component from using styled system props to CSS modules. It follows the instructions listed in the Primer React Styled Components to CSS Modules Refactoring Guide:

  • Verify VRT (Visual Regression Testing) Coverage (see existing e2e/components/TreeView.test.ts)
  • Ensure All Visual Changes Are Completed (confirmed by checking open issues related to TreeView)
  • Ensure the component accepts a className on the top DOM level only for styling from outside of primer/react
  • Ensure the component accepts a style prop for more dynamic styling like positioning
  • Add a feature flag for controlled rollout (staff shipping)
  • Ensure Component still accepts sx styling
BeforeAfter
light modes beforelight modes after
dark modes beforedark modes after

Changelog

New

  • Adds support for style prop to TreeView component

Changed

  • Updates TreeView component to support CSS Modules (behind feature flag)
  • Fixes minor test and documentation typos

Removed

N/A

Rollout strategy

Testing & Reviewing

These changes are easier to review with whitespace changes hidden.

Merge checklist

@ktraversktravers self-assigned this Nov 12, 2024
@changeset-bot

changeset-botBot commented Nov 12, 2024•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 919f426

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

This PR includes changesets to release 1 package
NameType
@primer/reactMinor

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

github-actionsBot commented Nov 12, 2024

Copy link
Copy Markdown
Contributor

👋 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-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Nov 12, 2024
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 12, 2024 17:32 Inactive
@github-actions

github-actionsBot commented Nov 12, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js99.12 KB (+0.44% 🔺)
packages/react/dist/browser.umd.js99.46 KB (+0.35% 🔺)

@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 12, 2024 19:24 Inactive
@ktravers
replace sx with Primer CSS utility classes where possible
1705d26
As of https://github.com/primer/react/blob/fca4ec98f2e3ef5e5c3298320e3ab2050593709c/contributor-docs/adrs/adr-016-css.md,
Primer has decided to move away from using styled system props and prefers CSS for
styling over styled system props due to performance concerns with `sx` props.
See linked ADR for full details.
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 12, 2024 19:35 Inactive
@ktravers
ktraversforce-pushed the ktravers/rm-sx-from-tree-view branch from 90debbb to c5fb3a7CompareNovember 12, 2024 19:39
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 12, 2024 19:41 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 12, 2024 20:01 Inactive
ktravers
ktravers commented Nov 13, 2024
View reviewed changes
Comment threadpackages/react/src/TreeView/TreeView.module.css

@ktraversktraversNov 13, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Copied styles directly from TreeView component, then migrated to CSS variables following this guide: https://primer.style/foundations/primitives/migrating#migrating-from-sx-props-primer-react

@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 13, 2024 01:55 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 13, 2024 02:25 Inactive
ktravers
ktravers commented Nov 13, 2024
View reviewed changes
Comment threadpackages/react/src/TreeView/TreeView.module.css
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 13, 2024 02:32 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5267 November 13, 2024 02:55 Inactive
@ktravers
ktraversforce-pushed the ktravers/rm-sx-from-tree-view branch from e72f964 to c28f0a4CompareNovember 13, 2024 02:56
@github-actionsgithub-actionsBot removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Nov 13, 2024
langermank
langermank reviewed Nov 13, 2024
View reviewed changes

@langermanklangermank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a few little things

Comment threadpackages/react/src/TreeView/TreeView.module.css Outdated
Comment threadpackages/react/src/TreeView/TreeView.module.css Outdated
jonrohan
jonrohan reviewed Nov 14, 2024
View reviewed changes
Comment thread.changeset/gentle-yaks-shake.md
Comment threadpackages/react/src/TreeView/TreeView.module.css Outdated
Comment threadpackages/react/src/TreeView/TreeView.module.css Outdated
Comment threadpackages/react/src/TreeView/TreeView.module.css Outdated
Comment threadpackages/react/src/TreeView/TreeView.module.css Outdated
ktraversand others added 2 commits November 13, 2024 22:30
@ktravers@jonrohan
use where with attribute selector
c1cf6e3
Co-authored-by: Jon Rohan <yes@jonrohan.codes>
@ktravers@langermank
use up-to-date color variable
a452922
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Nov 14, 2024
@github-actions

github-actionsBot commented Nov 14, 2024

Copy link
Copy Markdown
Contributor

👋 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!

@github-actionsgithub-actionsBot removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Nov 14, 2024
@github-actions

github-actionsBot commented Nov 14, 2024

Copy link
Copy Markdown
Contributor

👋 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!

@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Nov 14, 2024
@ktravers@jonrohan
Use where with attr selectors
1f6b6bc
Co-authored-by: Jon Rohan <yes@jonrohan.codes>
@github-actionsgithub-actionsBot added integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Nov 14, 2024
@github-actions

github-actionsBot commented Nov 14, 2024

Copy link
Copy Markdown
Contributor

👋 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!

@github-actions

github-actionsBot commented Nov 14, 2024

Copy link
Copy Markdown
Contributor

👋 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!

@github-actions

github-actionsBot commented Nov 14, 2024

Copy link
Copy Markdown
Contributor

👋 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!

@ktravers
Remove unnecessary workaround for dark high contrast mode
fbe385d
Variable problem referenced in e97cf72
was resolved by #5277.
@github-actions

github-actionsBot commented Nov 14, 2024

Copy link
Copy Markdown
Contributor

👋 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!

@ktravers

ktravers commented Nov 14, 2024

Copy link
Copy Markdown
ContributorAuthor

Taking back to draft while I look into new VRT failures...

@primer-integration

primer-integrationBot commented Nov 14, 2024

Copy link
Copy Markdown

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

@primer-integration

primer-integrationBot commented Nov 14, 2024

Copy link
Copy Markdown

🔴 golden-jobs completed with status failure.

langermank
langermank approved these changes Nov 14, 2024
View reviewed changes

@langermanklangermank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving the VRT updates! Before merging, we should confirm that the dotcom integration test failure isn't related (@jonrohan can take a look)

@jonrohan

jonrohan commented Nov 14, 2024

Copy link
Copy Markdown
Member

Yep these look like the same integration errors, not related to this pr

@primerprimerBot mentioned this pull request Nov 14, 2024
Merged
@ktraversktravers mentioned this pull request Nov 16, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonrohanjonrohanAwaiting requested review from jonrohan
1 more reviewer
@langermanklangermanklangermank approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ktraversktravers

Labels

component: TreeViewIssues related to the TreeView componentintegration-tests: failingChanges in this PR cause breaking changes in gh/ghstaffAuthor is a staff member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ktravers@jonrohan@langermank