-
Notifications
You must be signed in to change notification settings - Fork 81
feat(scrim): improve opacity and customizable token #13628
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
base: dev
Are you sure you want to change the base?
feat(scrim): improve opacity and customizable token #13628
Conversation
driskull
left a comment
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.
Just some comments! :)
| overflow-hidden; | ||
| animation: calcite-scrim-fade-in var(--calcite-internal-animation-timing-medium) ease-in-out; | ||
| background-color: var(--calcite-scrim-background, var(--calcite-color-transparent-scrim)); | ||
| opacity: var(--calcite-scrim-opacity, 0.7); |
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.
Should the fallback value of 0.7 use the theme() to get the token for the value so that it stays in sync with the token value?
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.
Should we be updating design tokens in a separate PR so that the conventional commit and changelog shows the update for the token?
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 have raised a separate PR for design tokens.
| overflow-hidden; | ||
| animation: calcite-scrim-fade-in var(--calcite-internal-animation-timing-medium) ease-in-out; | ||
| background-color: var(--calcite-scrim-background, var(--calcite-color-transparent-scrim)); | ||
| opacity: var(--calcite-scrim-opacity, theme("opacity.70")); |
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.
Once the token PR lands #13636, shouldn't there be be a token to use here for "color.transparent.scrim" or similar instead of the "opacity.70"?
|
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
Related Issue: #11666
Summary