Remove styled-system usage - misc components #6676
Conversation
🦋 Changeset detectedLatest commit: 8437a2c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
…ist's List component
mperrotti
force-pushed
the
mp/rm-sx/misc-components
branch
from
d760fd4 to
7f4b498
Compare
github-actions
bot
added
the
integration-tests: recommended
|
👋 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! |
size-limit report 📦
|
| variant?: 'small' | 'normal' | ||
| status: keyof typeof octiconMap | ||
| } & SxProp |
There was a problem hiding this comment.
I can put this back if we need. I accidentally got carried away at first because I misunderstood the scope of the issue.
There was a problem hiding this comment.
Pull Request Overview
This PR removes styled-system and styled-components dependencies from six components as part of a broader modernization effort. The changes migrate these components from styled-components to CSS modules with CSS custom properties and data attributes for styling.
Key Changes
- Replaced styled-component patterns with CSS modules and data attributes
- Converted component-specific styling to CSS custom properties for better performance and maintainability
- Updated component interfaces to remove
styled-systemspecific props
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Caret.tsx + Caret.module.css | Migrated from styled-components to CSS modules with custom properties for colors and borders |
| IssueLabelToken.tsx + IssueLabelToken.module.css | Converted complex styled-system logic to CSS modules with data attribute selectors |
| ToggleSwitch.tsx + ToggleSwitch.module.css | Replaced styled-components with CSS modules and comprehensive state management via data attributes |
| StateLabel.tsx + StateLabel.module.css | Simplified component by removing styled-system variants in favor of CSS modules |
| deprecated/ActionList/List.tsx | Updated to use BoxWithFallback and removed styled-components dependencies |
| Test snapshots | Updated to reflect new CSS class names and data attributes |
| const {borderColor} = getBorderColor(propsWithTheme) | ||
| const {borderWidth} = getBorderWidth(propsWithTheme) | ||
| const {size = 8, location = 'bottom'} = props | ||
| const {bg, borderColor, borderWidth, size = 8, location = 'bottom'} = props |
There was a problem hiding this comment.
The destructured bg, borderColor, and borderWidth props are not provided with fallback values, but are used as CSS custom properties on lines 91-93. This could result in undefined values being set as CSS properties. Consider providing fallback values or default values.
| const {bg, borderColor, borderWidth, size = 8, location = 'bottom'} = props | |
| const { | |
| bg = 'transparent', | |
| borderColor = 'currentColor', | |
| borderWidth = 1, | |
| size = 8, | |
| location = 'bottom' | |
| } = props |
Copilot uses AI. Check for mistakes.
|
|
||
| .CaretTriangle { | ||
| /* stylelint-disable-next-line primer/colors */ | ||
| fill: var(--caret-bg, var(--bgColor-default)); |
There was a problem hiding this comment.
The fallback CSS custom property --bgColor-default may not exist in all contexts. Consider using a more reliable fallback like a standard color value (e.g., #ffffff or currentColor).
| fill: var(--caret-bg, var(--bgColor-default)); | |
| fill: var(--caret-bg, var(--bgColor-default, currentColor)); |
Copilot uses AI. Check for mistakes.
| const listStyles: React.CSSProperties = { | ||
| fontSize: 'var(--text-body-size-medium, 14px)', | ||
| // 14px font-size * 1.428571429 = 20px line height |
There was a problem hiding this comment.
The hardcoded 14px fallback value duplicates the comment on line 112-113. Consider using a CSS custom property reference or extracting this value to a constant to maintain consistency.
| const listStyles: React.CSSProperties = { | |
| fontSize: 'var(--text-body-size-medium, 14px)', | |
| // 14px font-size * 1.428571429 = 20px line height | |
| const LIST_FONT_SIZE_FALLBACK = '14px' | |
| const listStyles: React.CSSProperties = { | |
| fontSize: `var(--text-body-size-medium, ${LIST_FONT_SIZE_FALLBACK})`, | |
| // LIST_FONT_SIZE_FALLBACK * 1.428571429 = 20px line height |
Copilot uses AI. Check for mistakes.
| @@ -1,3 +1,114 @@ | |||
| .IssueLabel { | |||
There was a problem hiding this comment.
Copilot detected a code snippet with 50+ occurrences. See search results for more details.
Matched Code Snippet
-label-r: 153;
--label-g: 153;
--label-b: 153;
--label-h: 0;
--label-s: 0;
--label-l: 60;
--perceived-lightness: calc(
((var(--label-r) * 0.2126) + (var(--label-g) * 0.7152) + (var(--label-b) * 0.0722)) / 255
);
--lightness-switch: max(0, min(calc((var(--perceived-lightness) - var(--lightness-threshold)) * -1000), 1));
position: relative;
Copilot uses AI. Check for mistakes.
| @@ -1,3 +1,114 @@ | |||
| .IssueLabel { | |||
There was a problem hiding this comment.
Copilot detected a code snippet with 50 occurrences. See search results for more details.
Matched Code Snippet
-lightness-threshold: 0.453;
--border-threshold: 0.96;
--border-alpha: max(0, min(calc((var(--perceived-lightness) - var(--border-threshold)) * 100)
Copilot uses AI. Check for mistakes.
| @@ -1,3 +1,114 @@ | |||
| .IssueLabel { | |||
There was a problem hiding this comment.
Copilot detected a code snippet with 50 occurrences. See search results for more details.
Matched Code Snippet
border-color: hsla(var(--label-h), calc(var(--label-s) * 1%), calc((var(--label-l) - 25) * 1%), var(--border-
Copilot uses AI. Check for mistakes.
| @@ -1,3 +1,114 @@ | |||
| .IssueLabel { | |||
There was a problem hiding this comment.
Copilot detected a code snippet with 50+ occurrences. See search results for more details.
Matched Code Snippet
-lightness-threshold: 0.6;
--background-alpha: 0.18;
--border-alpha: 0.3;
--lighten-by: calc(((var(--lightness-threshold) - var(--perceived-lightness)) * 100) * var(--lightness-switch)
Copilot uses AI. Check for mistakes.
| @@ -1,3 +1,114 @@ | |||
| .IssueLabel { | |||
There was a problem hiding this comment.
Copilot detected a code snippet with 50+ occurrences. See search results for more details.
Matched Code Snippet
var(--label-h),
calc(var(--label-s) * 1%),
calc((var(--label-l) + var(--lighten-by)) * 1%),
var(--border-alpha)
)
Copilot uses AI. Check for mistakes.
| @@ -1,3 +1,114 @@ | |||
| .IssueLabel { | |||
There was a problem hiding this comment.
Copilot detected a code snippet with 13 occurrences. See search results for more details.
Matched Code Snippet
image: linear-gradient(rgba(0, 0, 0, 0.15), rgba(0, 0, 0, 0.15)),
linear-gradient(
rgb(var(--label-r), var(--label-g), var(--label-b)),
rgb(var(--label-r), var(--label-g), var(--label-b))
);
box-shadow:
Copilot uses AI. Check for mistakes.
| prop: 'status', | ||
| variants: { | ||
| issueClosed: { | ||
| backgroundColor: 'done.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-done-emphasis, transparent)', | ||
| }, | ||
| issueClosedNotPlanned: { | ||
| backgroundColor: 'neutral.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-neutral-emphasis, transparent)', | ||
| }, | ||
| pullClosed: { | ||
| backgroundColor: 'closed.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-closed-emphasis, transparent)', | ||
| }, | ||
| pullMerged: { | ||
| backgroundColor: 'done.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-done-emphasis, transparent)', | ||
| }, | ||
| pullQueued: { | ||
| backgroundColor: 'attention.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-attention-emphasis, transparent)', | ||
| }, | ||
| issueOpened: { | ||
| backgroundColor: 'open.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-open-emphasis, transparent)', | ||
| }, | ||
| pullOpened: { | ||
| backgroundColor: 'open.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-open-emphasis, transparent)', | ||
| }, | ||
| draft: { | ||
| backgroundColor: 'neutral.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-neutral-emphasis, transparent)', | ||
| }, | ||
| issueDraft: { | ||
| backgroundColor: 'neutral.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-neutral-emphasis, transparent)', | ||
| }, | ||
| unavailable: { | ||
| backgroundColor: 'neutral.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-neutral-emphasis, transparent)', | ||
| }, | ||
| open: { | ||
| backgroundColor: 'open.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-open-emphasis, transparent)', | ||
| }, | ||
| closed: { | ||
| backgroundColor: 'done.emphasis', | ||
| color: 'fg.onEmphasis', | ||
| boxShadow: 'var(--boxShadow-thin, inset 0 0 0 1px) var(--borderColor-done-emphasis, transparent)', | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| const sizeVariants = variant({ | ||
| prop: 'variant', | ||
| variants: { | ||
| small: { | ||
| paddingX: 2, | ||
| paddingY: 1, | ||
| fontSize: 0, | ||
| }, | ||
| normal: { | ||
| paddingX: '12px', | ||
| paddingY: 2, | ||
| fontSize: 1, | ||
| }, | ||
| }, | ||
| }) |
| // TODO: When rem-based spacing on a 4px scale lands, replace hardcoded '20px' | ||
| lineHeight: '20px', |
There was a problem hiding this comment.
should we do var(--base-size-20) here now?
There was a problem hiding this comment.
Technically, yes, but I don't really see the value in changing it - especially for a deprecated component 🤷
Relates to https://github.com/github/primer/issues/5422
The primary purpose of these changes is to remove usage of
styled-systemandstyled-componentsutilities from the following components:Changelog
New
Changed
styled-systemandstyled-componentsdependencies from the components listed above.sxprop from IssueLabelToken (can be brought back if needed)Removed
Rollout strategy
Testing & Reviewing
Merge checklist