-
Notifications
You must be signed in to change notification settings - Fork 638
chore(Octicon): remove sx #6945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 4ac195e 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 |
👋 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! |
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/3844 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the sx
prop from the Octicon component as part of a migration away from styled-components. The change replaces inline sx
styling with CSS modules for improved performance and consistency.
Key Changes
- Removed
sx
prop support from the main Octicon component in@primer/react
- Added a styled-components wrapper for backward compatibility in
@primer/styled-react
- Migrated all usage from
sx
props to CSS classes throughout the codebase
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/react/src/Octicon/Octicon.tsx | Simplified component by removing styled-components and sx prop support |
packages/styled-react/src/components/Octicon.tsx | Added styled wrapper to maintain sx prop compatibility for legacy package |
packages/styled-react/src/deprecated.tsx | Updated export to use local Octicon wrapper instead of deprecated one |
Multiple story files | Replaced sx props with CSS module classes for styling |
Multiple CSS module files | Added CSS classes to replace inline sx styling |
packages/react/src/StateLabel/StateLabel.tsx | Replaced width prop with data attribute for conditional styling |
Co-authored-by: Copilot <[email protected]>
👋 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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
packages/react/src/experimental/SelectPanel2/SelectPanel.examples.stories.module.css
Outdated
Show resolved
Hide resolved
packages/react/src/experimental/SelectPanel2/SelectPanel.examples.stories.module.css
Outdated
Show resolved
Hide resolved
👋 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! |
🟢 ci completed with status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pew pew pew
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the visual diffs look suspicious, otherwise all good
Approving in advance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoop, this diff looks suspicious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved it with @langermank! We had some styles that weren't being correctly applied in styles components, so when I migrated I inadvertently "fixed" them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah perfect
👋 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! |
Co-authored-by: Copilot <[email protected]>
Removes sx from Octicon component and exports styled wrapper from styled-react
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist