Skip to content
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 #8899

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Mar 7, 2024

Related Issue: #7180

Summary

Adds the following component tokens (CSS props):

  • --calcite-slider-fill-color
  • --calcite-slider-histogram-accent-color
  • --calcite-slider-histogram-background-color
  • --calcite-slider-handle-border-color-hover
  • --calcite-slider-handle-border-color
  • --calcite-slider-handle-color-active
  • --calcite-slider-handle-color-hover
  • --calcite-slider-handle-color
  • --calcite-slider-precise-handle-color-hover
  • --calcite-slider-precise-handle-color
  • --calcite-slider-handle-shadow-hover
  • --calcite-slider-handle-shadow
  • --calcite-slider-text-color
  • --calcite-slider-text-size
  • --calcite-slider-tick-border-color
  • --calcite-slider-tick-color
  • --calcite-slider-track-color

@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 7, 2024
@jcfranco jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Mar 13, 2024
@jcfranco
Copy link
Member Author

@SkyeSeitz @ashetland I originally had a prop for styling the min/max labels for a couple of specific ranged-histogram use cases, but removed them to ask whether we wanted to update Figma accordingly or to use the same text color for all tick labels?

Design spec

Screenshot 2024-03-13 at 2 57 23 AM

Implementation

Screenshot 2024-03-13 at 3 07 42 AM

For additional context, the edge label styling is only applicable to sliders + range + histogram + precise/label-handles (introduced here). cc @eriklharper

@jcfranco jcfranco marked this pull request as ready for review March 13, 2024 10:13
@jcfranco jcfranco requested a review from a team as a code owner March 13, 2024 10:13
@ashetland
Copy link
Contributor

ashetland commented Mar 13, 2024

It looks like the font weight has decreased too on the min/max labels. I think we should keep all the labels the same 12px/500/color.text.2 as a default so they are all consistent across modes. Providing css vars to customize the color of min/max labels and handle labels separately though makes sense to me.

Copy link
Contributor

@alisonailea alisonailea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a few questions about box-shadows.

.handle {
box-shadow: 0 0 0 3px var(--calcite-color-brand) inset;
background-color: var(--calcite-slider-handle-color-active, var(--calcite-color-brand));
box-shadow: 0 0 0 3px var(--calcite-slider-handle-border-color-hover, var(--calcite-color-brand)) inset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be box-shadow: var(--calcite-slider-handle-shadow, 0 0 0 3px var(--calcite-slider-handle-border-color-hover, var(--calcite-color-brand)) inset); ?
this applies to other box-shadows on the handle

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had considered something like that, but to a consumer, the inset box-shadow is used as a border. From that perspective, I think it'd be better to only expose color props for the handle's "border". This might affect other similar shadow props. I know I'll have to update switch to follow suit. cc @ashetland @macandcheese

.thumb--active {
.handle {
@apply bg-brand;
box-shadow: 0 0 8px 0 rgb(0 0 0 / 16%); // shadow 1 press
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this box-shadow need to be added back?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I unintentionally removed it because it seemed out of place compared to other shadows on the handle, but after chatting w/ @ashetland, we may consider dropping it since it's currently applied only when the thumb is active/pressed.

@jcfranco jcfranco requested a review from alisonailea March 15, 2024 21:29
* @prop --calcite-slider-handle-color-hover: Specifies the color of the handle when hovered.
* @prop --calcite-slider-handle-color: Specifies the color OF the handle.
* @prop --calcite-slider-handle-extension-color-hover: Specifies the color of the handle extension when hovered.
* @prop --calcite-slider-handle-extension-color: Specifies the color of the handle extension.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could use the precise nomenclature that displays this? Either in prop name or description.

* These properties can be overridden using the component's tag as selector.
*
* @prop --calcite-slider-fill-color: Specifies the color of the fill.
* @prop --calcite-slider-graph-accent-color: Specifies the accent color of the graph.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use the histogram prop nomenclature? (Or, could histogram prop use graph, etc.,)

jcfranco added a commit that referenced this pull request Mar 19, 2024
**Related Issue:** #7180

## Summary

Drops shadow CSS prop in favor of border for thumb border effect.

Removes the following component tokens (CSS props):

* `--calcite-switch-handle-shadow`
* `--calcite-block-section-switch-handle-shadow`
* `--calcite-block-section-switch-handle-shadow-hover`

Stems from
#8899 (comment).
@jcfranco jcfranco merged commit 9a91503 into epic/7180-component-tokens Mar 20, 2024
5 checks passed
@jcfranco jcfranco deleted the jcfranco/7180-add-component-tokens-to-slider branch March 20, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants