Add new tokens for border radius and Heading #4401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RobinCsl
commented
Jun 28, 2024
packages/orbit-design-tokens/src/dictionary/definitions/global/borderRadius.json
Outdated
Show resolved
Hide resolved
RobinCsl
commented
Jun 28, 2024
RobinCsl
commented
Jun 28, 2024
|
Storybook staging is available at https://kiwicom-orbit-rcsl-add-new-tokens.surge.sh |
|
Size Change: +386 B (+0.09%) Total Size: 446 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
10ea1e9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e626ce6c.orbit.pages.dev |
| Branch Preview URL: | https://rcsl-add-new-tokens.orbit.pages.dev |
RobinCsl
force-pushed
the
rcsl/add-new-tokens
branch
2 times, most recently
from
dfd0617 to
3c223df
Compare
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
from
3c223df to
6c7d0bc
Compare
DSil
force-pushed
the
rcsl/add-new-tokens
branch
2 times, most recently
from
3c223df to
893c867
Compare
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
8 times, most recently
from
d0692fb to
96cd3df
Compare
|
Hello, I updated border radius values, updated visual screenshots and snapshots for the tests, but still getting failed unit test - https://github.com/kiwicom/orbit/actions/runs/10038370965/job/27740127403?pr=4401 I guess it's caused that |
|
Hmm I am not sure I understand your problem, and it seems it relies in some wrong assumptions / statements:
This is not true! The tokens did not change values. New tokens were added to replace old ones that will be removed soon (hence the deprecation). The old tokens must not change their values. Otherwise, it is a breaking change. More than that, the test is correct. If the token did not change its value (as expected), it still computes to 3px. Now… how is the snapshot expecting 4px? How did this change happen? |
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
from
96cd3df to
5ec5b41
Compare
I described it in a confusing way, but I can't explain myself how the received value can be The values weren't changed for old names, but:
That was for testing purposes, sorry. 🙈 The unit test is failing without these updated snapshots anyway. |
That is expected. It is a visual change made by the designers that we adopt.
Well, there were two failing and now there is only one 😄 The tokens one was failing and it should not. Now, the one failing is on Box. If we look at the definition of Box, we see that it expects the The Box component is a special component, very tied to the tokens. So in my opinion we should:
This way we avoid the breaking change for now and we can remove the prop values when we remove the tokens (preventing inconsistencies) |
|
Thank you for your response. I get the reasons now. This statement ⬇️ is not clear to me:
Which props and where (file) do you mean? Can you provide more details, please? Btw, I added new types to |
sarkaaa
reviewed
Jul 23, 2024
packages/orbit-design-tokens/src/dictionary/definitions/global/borderRadius.json
Show resolved
Hide resolved
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
from
7bc3c19 to
7c64d0d
Compare
sarkaaa
changed the title
|
Jira ticket: https://kiwicom.atlassian.net/browse/FEPLT-2050 |
DSil
force-pushed
the
rcsl/add-new-tokens
branch
5 times, most recently
from
e7b0a04 to
1cb1404
Compare
The change ensures we're correctly transforming the dot notation to access object properties when numbers are used as keys: 'foundations.borderRadius.50' is incorrect JavaScript; 'foundations.borderRadius["50"]' is valid JavaScript.
New tokens: none, 50, 100, 150, 300, 400 Deprecated tokens: small, normal, large
New classes: rounded-50, rounded-100, rounded-150, rounded-300, rounded-400 Deprecated: rounded-small, rounded-normal, rounded-large
New possible values: "none", "full", "50", "100", "150", "300", "400" Deprecated: "small", "normal", "large", "circle"
BREAKING CHANGE: deprecated fontSize, lineHeight and textWeight tokens for Heading were removed. The replacement tokens can be used. Check the migration guide for more information.
BREAKING CHANGE: deprecated leading-heading class was removed.
Tests were deleted because they relied on deprecated tokens and we agreed not to test like this a while ago. BREAKING CHANGE: `type="title1"` should be replaced by `type="title0"` to avoid visual breaking change.
DSil
force-pushed
the
rcsl/add-new-tokens
branch
from
1cb1404 to
d96968a
Compare
DSil
force-pushed
the
rcsl/add-new-tokens
branch
from
10ea1e9 to
beca013
Compare
DSil
changed the title
DSil
approved these changes
Aug 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FEPLT-2050
What remains to be done:
rounded-50classes not being part of thepackages/orbit-components/lib/tailwind.cssgenerated file for some reason (might be a step I missed);Commits:
docs: add changelog draft for v16
feat: update Tailwind classes for new border radius tokens
through the new transform.
Changes generated by running: 'node ../transforms/tokens-v16.mjs' from
the 'src' folder in packages/orbit-components.
BREAKING CHANGE: All the classes using normal border radius will now see
a 1px larger border radius through the Orbit Design Token update and
mapping between old tokens and new.
feat(tokens): add new tokens for border-radius
Slack: https://skypicker.slack.com/archives/C7T7QG7M5/p1719493641111859
chore(tokens): allow numbers as keys in tokens
The change ensures we're correctly transforming the dot notation to access
object properties when numbers are used as keys:
'foundations.borderRadius.50' is incorrect JavaScript;
'foundations.borderRadius["50"]' is valid JavaScript.