-
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
Update border-radii design tokens #980
Conversation
Hey @robinmetral, Thanks! |
🦋 Changeset detectedLatest commit: e9c4e5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/7YyGMriodJdYc1w5s92ajbwk2m4X |
800be19
to
6950a4d
Compare
This also bundles all component renames into a single codemod (typography components and NotificationBanner)
6950a4d
to
f1bcc8a
Compare
Codecov Report
@@ Coverage Diff @@
## next #980 +/- ##
==========================================
+ Coverage 91.68% 91.70% +0.02%
==========================================
Files 165 165
Lines 3077 3074 -3
Branches 782 754 -28
==========================================
- Hits 2821 2819 -2
Misses 227 227
+ Partials 29 28 -1
|
2a97481
to
494c4fa
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.
See also the deploy preview and Chromatic to review this visually ✨
packages/circuit-ui/cli/migrate/__testfixtures__/theme-border-radius.output.js
Show resolved
Hide resolved
renameFactory(j, root, 'mega', 'bit'); | ||
renameFactory(j, root, 'giga', 'byte'); | ||
renameFactory(j, root, 'tera', 'byte'); | ||
renameFactory(j, root, 'peta', 'kilo'); |
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.
Note: this doesn't replace this pattern:
const elementStyles = css`
border-radius: ${p => p.theme.borderRadius.tera};
`;
I've seen it used more regularly recently, I'm not sure if I should extend the codemod to fit it?
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.
Yeah, let's extend the codemod to cover this case. Would you like to pair on this?
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'd love to!
3afc440
to
e9c4e5c
Compare
Purpose
Updates border radii to match Figma. Generally the mapping is:
1 the value isn't part of design tokens anymore but some 1px radii were hardcoded in Circuit where it made sense, see comments below
2 6px radii were generally switched to 8px after consulting with design
Approach and changes
FIXME
comments in the codebase (mostly border-radius related)<Plus />
icon (it had a hardcoded#000
color)ℹ️ the upgrade of Circuit's
@sumup/design-tokens
peer dependency will be done in a separate PR, we need to release it firstDefinition of done