New shadow tokens #4439
Merged
New shadow tokens #4439
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
|
Storybook staging is available at https://kiwicom-orbit-marco-shadow-tokens.surge.sh |
|
Size Change: +190 B (+0.04%) Total Size: 446 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
ae7cc33
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://38011c1c.orbit.pages.dev |
| Branch Preview URL: | https://marco-shadow-tokens.orbit.pages.dev |
mvidalgarcia
force-pushed
the
marco/shadow-tokens
branch
from
f3aeccf to
3cd93bf
Compare
DSil
force-pushed
the
rcsl/add-new-tokens
branch
from
e7b0a04 to
1cb1404
Compare
mvidalgarcia
force-pushed
the
marco/shadow-tokens
branch
2 times, most recently
from
96f90fa to
38af8f1
Compare
DSil
reviewed
Aug 6, 2024
There was a problem hiding this comment.
We should also update the unit tests that assert that the elevation prop applies the correct values to the CSS variable
There was a problem hiding this comment.
We have this file called SCHEMA.md under docs in this package that should be updated (and also for the other tokens 🫣)
There was a problem hiding this comment.
I updated it (only shadows)
DSil
reviewed
Aug 6, 2024
| @@ -166,7 +166,7 @@ const PopoverContentWrapper = ({ | |||
| "h-auto w-full", | |||
| "z-[1000]", | |||
| "box-border", | |||
| "shadow-raised-reverse", | |||
| "shadow-level3-reverse", | |||
| "bg-elevation-raised-reverse", | |||
There was a problem hiding this comment.
How are we addressing these? This shadow will disappear but the bg-color remains? Should we deprecate it as well and just use the color directly? In this case, it would be just bg-white-normal, no?
There was a problem hiding this comment.
Yeah, backgrounds are still missing.. I need to tackle this and probably just use the replacement (white normal as you suggested).
There was a problem hiding this comment.
I see you kept the tokens or renamed them accordingly. I'm ok with that, but I had suggested to completely get rid of those tokens and use the palette directly.
In this case we would use bg-white-normal instead of bg-elevation-raised-reverse that has the value of white.
And btw, raised reverse is deprecated, so this needs to be updated to bg-level3-reverse, right?
There was a problem hiding this comment.
I'm removing tokens and using bg-white-normal
mvidalgarcia
force-pushed
the
marco/shadow-tokens
branch
from
38af8f1 to
b2c1f90
Compare
DSil
reviewed
Aug 8, 2024
| "category": "Colors", | ||
| "version": "9.0.0" | ||
| }, | ||
| "elevationActionBoxShadow": { |
There was a problem hiding this comment.
I have just realized we now have two levels of deprecations 🤦♂️
The token boxShadowAction is deprecated and suggests elevationActionBoxShadow as replacement. But now elevationActionBoxShadow is also deprecated and suggests elevationLevel1BoxShadow as replacement.
I am not entirely sure about how this will impact the breaking change, but for instance, I see the boxShadowAction (the double deprecated one) is currently being used here… 😬 The same for modal-scrolled and modal defined right below: they're also using doubly deprecated tokens.
So, yes, I honestly don't know what to do with these. But I'd recommend going through all the tokens (even the older ones) and replace them for the new ones. For example, in that case, replace that switch definition by the correct elevationLevel1BoxShadow token (although I'm also not sure about the consequences of such change…)
I did the quick exercise of completely deleting the tokens and the only place where the build failed was the file mentioned above, but I am not 100% sure this is all the confidence we need.
| @@ -166,7 +166,7 @@ const PopoverContentWrapper = ({ | |||
| "h-auto w-full", | |||
| "z-[1000]", | |||
| "box-border", | |||
| "shadow-raised-reverse", | |||
| "shadow-level3-reverse", | |||
| "bg-elevation-raised-reverse", | |||
There was a problem hiding this comment.
I see you kept the tokens or renamed them accordingly. I'm ok with that, but I had suggested to completely get rid of those tokens and use the palette directly.
In this case we would use bg-white-normal instead of bg-elevation-raised-reverse that has the value of white.
And btw, raised reverse is deprecated, so this needs to be updated to bg-level3-reverse, right?
Comment on lines
302
to
294
| "raised-reverse": { | ||
| "background": { | ||
| "type": "color", | ||
| "category": "elevation", | ||
| "value": "{foundation.palette.white.normal}", | ||
| "deprecated": true, | ||
| "deprecated-replace": "{foundation.palette.white.normal}", | ||
| "deprecated-version": "9.0.0" |
There was a problem hiding this comment.
Following my previous comment…
I see this is being deprecated with the replacement pointing to palette.white.normal. But then the level3-reverse background color is also being created, with the value of palette.white.normal.
So I think we are not going through the correct approach. We either:
- Use
level3-reverseas a replacement here and usepalette.white.normalas a value there
OR
- Use
palette.white.normalas a replacement here but do not definelevel3-reverseat all
I personally prefer the second option, as it will remove the overdefinition of tokens entirely. But I am open for discussing (and accepting) the first option as well
There was a problem hiding this comment.
Ok, gotcha, I'll remove the background tokens and adapt things accordingly. For some reason, I thought we wanted to keep backgrounds as well but I see they are barely used, so there's no point.
Comment on lines
122
to
125
| <Box borderRadius="small" elevation="action">"small" borderRadius</Box> | ||
| <Box borderRadius="normal" elevation="raised">"normal" borderRadius</Box> | ||
| <Box borderRadius="large" elevation="raisedReverse">"large" borderRadius</Box> | ||
| <Box borderRadius="circle" elevation="overlay">"circle" borderRadius</Box> |
There was a problem hiding this comment.
Given that the content is no longer just about border radius…
Suggested change
| <Box borderRadius="small" elevation="action">"small" borderRadius</Box> | |
| <Box borderRadius="normal" elevation="raised">"normal" borderRadius</Box> | |
| <Box borderRadius="large" elevation="raisedReverse">"large" borderRadius</Box> | |
| <Box borderRadius="circle" elevation="overlay">"circle" borderRadius</Box> | |
| <Box borderRadius="small" elevation="action">…</Box> | |
| <Box borderRadius="normal" elevation="raised">…</Box> | |
| <Box borderRadius="large" elevation="raisedReverse">…</Box> | |
| <Box borderRadius="circle" elevation="overlay">…</Box> |
And same for the example below
DSil
force-pushed
the
rcsl/add-new-tokens
branch
2 times, most recently
from
10ea1e9 to
beca013
Compare
|
I have merged the base branch to master and now I see there are some conflicts. Let me know if you wanna pair so we are sure nothing is missed 🤞 |
- New tokens: level1, level2, level3, level3-reverse, level4 - Deprecated tokens: action, action-active, raised, raised-reverse, overlay
New classes: - shadow-level1, shadow-level2, shadow-level3, shadow-level3-reverse, shadow-level4 Deprecated: - shadow-action, shadow-action-active, shadow-raised, shadow-raised-reverse, shadow-overlay - bg-elevation-action, bg-elevation-action-active, bg-elevation-raised, bg-elevation-raised-reverse, bg-elevation-overlay
mvidalgarcia
force-pushed
the
marco/shadow-tokens
branch
from
b2c1f90 to
ae7cc33
Compare
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.
https://skypicker.slack.com/archives/C7T7QG7M5/p1721117646848839?thread_ts=1720790040.455089&cid=C7T7QG7M5
Closes FEPLT-2064