Skip to content
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

Merged
merged 8 commits into from
Feb 15, 2022

Conversation

amelako
Copy link
Contributor

@amelako amelako commented Feb 14, 2022

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

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@sumup-clark
Copy link

sumup-clark bot commented Feb 14, 2022

Hey @amelako,
we are super excited that you are contributing! 🎉There's one more thing you need to do. Please accept our Contributor License Agreement. It helps you and us to collaborate on clear terms and focus on what we love most: code.

Thanks!

@vercel
Copy link

vercel bot commented Feb 14, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/CGbhppwsB4fGUT6gyaBhBeFAnZLe
✅ Preview: https://oss-circuit-ui-git-update-design-token-colors-sumup.vercel.app

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #1431 (2334b23) into main (60d57a8) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
packages/circuit-ui/components/Badge/Badge.tsx 100.00% <ø> (ø)
packages/circuit-ui/components/Body/Body.tsx 97.22% <ø> (ø)
...ages/circuit-ui/components/BodyLarge/BodyLarge.tsx 100.00% <ø> (ø)
packages/circuit-ui/components/Button/Button.tsx 93.84% <ø> (ø)
...ckages/circuit-ui/components/Checkbox/Checkbox.tsx 96.96% <ø> (ø)
...es/circuit-ui/components/ImageInput/ImageInput.tsx 86.45% <ø> (ø)
...cuit-ui/components/InlineMessage/InlineMessage.tsx 94.73% <ø> (ø)
packages/circuit-ui/components/Input/Input.tsx 90.76% <ø> (ø)
...mponents/NotificationInline/NotificationInline.tsx 92.72% <ø> (-0.13%) ⬇️
...components/NotificationToast/NotificationToast.tsx 89.36% <ø> (-0.23%) ⬇️
... and 5 more

Copy link
Contributor Author

@amelako amelako left a 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:

  1. 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?
  2. 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 :)

@robinmetral
Copy link
Contributor

  1. 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?

Makes sense, let's keep it 👍

  1. 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.

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).

@changeset-bot
Copy link

changeset-bot bot commented Feb 14, 2022

🦋 Changeset detected

Latest commit: 2334b23

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

This PR includes changesets to release 2 packages
Name Type
@sumup/design-tokens Minor
@sumup/circuit-ui Major

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

@connor-baer
Copy link
Member

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.

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).

You could add the new variants already and deprecate the old ones, would make the migration easier.

@robinmetral
Copy link
Contributor

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.

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).

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 🙂

@robinmetral
Copy link
Contributor

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.

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).

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

Copy link
Contributor

@robinmetral robinmetral left a 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)

.changeset/seven-books-tan.md Outdated Show resolved Hide resolved
.changeset/wicked-timers-bake.md Outdated Show resolved Hide resolved
Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

💯

@robinmetral
Copy link
Contributor

The Chromatic change is unexpected, I'm re-running the job to see if there was a hiccup

@robinmetral robinmetral changed the title Add design token colors (info, confirm, notify and alert) Use new semantic color names (info, confirm, notify and alert) Feb 15, 2022
@amelako amelako merged commit 3f9585a into main Feb 15, 2022
@amelako amelako deleted the update-design-token-colors branch February 15, 2022 11:52
@github-actions github-actions bot mentioned this pull request Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants