-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat(slider): add component tokens #10716
base: dev
Are you sure you want to change the base?
Conversation
…nto dit13711/7180-add-tokens-to-slider
we are not adding tokens for outline-color and I don't think this component needs stateful tokens |
@alisonailea |
We may in the future. For now, here's a list of our tokenizable styles |
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.
Review the component for shared design patterns. It's more important that tokens define our design patterns than match the structure of the component. Also make sure tokens can be applied in the various host states including disabled.
@@ -140,15 +158,15 @@ | |||
.handle-extension { | |||
@apply w-0.5; | |||
block-size: var(--calcite-slider-handle-extension-height); | |||
background-color: var(--calcite-color-text-3); | |||
background-color: var(--calcite-slider-handle-extension-color, var(--calcite-color-text-3)); | |||
} | |||
|
|||
&:hover { | |||
.handle { | |||
box-shadow: 0 0 0 3px var(--calcite-color-brand) inset; |
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.
This line, line 169, line 178, line 242, and line 250 should all have a shared 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.
Thanks, for the shared token since it spans different elements and even states, what would be a good name be something like? Something like --calcite-slider-accent-color
?
Also wondering about the extension tokens (line 169 and 178) being shared across states, the way it works without tokens is that extension is a separate color then gets highlighted with --calcite-color-brand
on hover and focus.
top-0 | ||
h-0.5; | ||
background-color: var(--calcite-slider-track-fill-color, var(--calcite-color-brand)); |
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.
this might also actually need to be changed to be a shared token with the lines above.
inset-block-start: -2px; | ||
pointer-events: none; | ||
margin-inline-start: calc(-1 * theme("margin[0.5]")); | ||
} | ||
|
||
.tick--active { | ||
@apply bg-brand; | ||
background-color: var(--calcite-slider-tick-selected-color, var(--calcite-color-brand)); |
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.
also this one.
Related Issue: #7180
Summary
Add tokens for the slider component, e2e, and custom-theme chromatic tests. (WIP)
Slider Tokens
--calcite-slider-text-color
: Specifies the component's text color.--calcite-slider-track-color
: Specifies the component's track color.--calcite-slider-track-fill-color
: Specifies the component's fill track color.--calcite-slider-handle-fill-color
: Specifies the component's handle fill color.--calcite-slider-handle-extension-color
: Specifies the component's handle extension color.--calcite-slider-tick-color
: Specifies the component's tick color.--calcite-slider-tick-border-color
: Specifies the component's tick border color.--calcite-slider-tick-selected-color
: Specifies the component's tick color when in selected range.--calcite-slider-graph-color
: Specifies the component's graph color.