-
Notifications
You must be signed in to change notification settings - Fork 129
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
Use new semantic color names (info, confirm, notify and alert) #1431
Conversation
Hey @amelako, Thanks! |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/CGbhppwsB4fGUT6gyaBhBeFAnZLe |
Codecov Report
@@ Coverage Diff @@
## main #1431 +/- ##
==========================================
- Coverage 92.34% 92.33% -0.01%
==========================================
Files 193 193
Lines 3892 3889 -3
Branches 1215 1213 -2
==========================================
- Hits 3594 3591 -3
Misses 279 279
Partials 19 19
|
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.
@robinmetral updated most of the places in circuit ui as well.
There are few things I would like to clear up:
- Old Notification component and InlineMessage have the danger/success/warning variants. I did update the colors, which now contradicts with the naming a bit. But I don't see the point of changing the variant naming to match since these are gonna be deprecated anyway. Let me know if you agree?
- Body component has variants success and error as well as Badge success, warning and danger. I just changed the colors usage there to match the new ones. Let me know if you have any thoughts on the variants cause that would mean a bigger change then.
Let me know if you have any other thoughts or ideas. Thank :)
Makes sense, let's keep it 👍
Ah, that's true. Let's keep it for now and handle the renaming in the next major (maybe you can add a comment in there to remind us). |
packages/circuit-ui/components/ImageInput/__snapshots__/ImageInput.spec.tsx.snap
Outdated
Show resolved
Hide resolved
packages/circuit-ui/components/ValidationHint/__snapshots__/ValidationHint.spec.tsx.snap
Outdated
Show resolved
Hide resolved
🦋 Changeset detectedLatest commit: 2334b23 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 |
You could add the new variants already and deprecate the old ones, would make the migration easier. |
That's true—let's handle it in a separate PR to keep this one's scope small 🙂 |
Nested quote replies FTW |
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.
Also needs a couple changesets (for Circuit and the tokens), but looks good to go ✨
+ I like Connor's idea of immediately adding the new variant names to the Body
, BodyLarge
and Badge
, and deprecating the old variant names. Let's split it into a separate PR
++ You could also make this another entry in the draft migration guide from v4.x to v5 (since that's when we'll remove the old variant names)
packages/circuit-ui/components/NotificationInline/NotificationInline.tsx
Show resolved
Hide resolved
71c13f9
to
468bca2
Compare
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.
💯
The Chromatic change is unexpected, I'm re-running the job to see if there was a hiccup |
Purpose
In order to match the new naming of colors and variants in the Notification components, added info, confirm, notify and alert colors to design tokens.
Approach and changes
Added info, alert, notify and confirm types in design tokens.
Updated light.ts with the new colors.
Replaced the warning, success and danger color usages in circuit ui with the new ones.
Definition of done