-
Notifications
You must be signed in to change notification settings - Fork 537
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
feat(SelectPanel2): Convert SelectPanel2 to CSS modules #5325
Conversation
🦋 Changeset detectedLatest commit: 09de060 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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! |
size-limit report 📦
|
…am/select-panel-2-css-modules
…ub.com/primer/react into hussam-i-am/select-panel-2-css-modules
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.
Mostly LGTM, just one question around classNames.
I'm wondering if we want to rethink shipping under a FF, I do see this being pulled in from /experimental in dotcom unless I'm reading this wrong?
👋 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! |
👋 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 Readded the sx props and update classNames to pass through from props |
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/352234 |
🟢 golden-jobs 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.
Looks good! one non-blocking suggestion
<Box sx={{display: 'flex', alignItems: 'center', gap: 2}}> | ||
<Checkbox id={checkboxId} sx={{marginTop: 0}} {...props} /> | ||
<InputLabel htmlFor={checkboxId} sx={{fontSize: 0}}> | ||
<Box |
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.
nit: should we render theses Box
es as native divs when enabled?
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 believe it defaults to a div and doesn't need it in those cases
Changelog
New
Changed
SelectPanel
to CSS modules without feature flag since it is only used internally and does not currently accept ansx
orclassName
propRemoved
Rollout strategy
Testing & Reviewing
Merge checklist