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(colorslider): S2 migration #3424

Open
wants to merge 4 commits into
base: spectrum-two
Choose a base branch
from

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Dec 2, 2024

Description

CSS-1028

S2 colorslider migration

This migrates the colorslider component to S2. Custom properties have been remapped per the design spec.

Before After
--spectrum-gray-900-rgb --spectrum-gray-1000-rgb

Validation steps

  1. Open the URL for the PR.
  2. Navigate to the colorslider component and verify no regressions have occurred.

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@cdransf cdransf added blocked See description and comments for what is blocking this issue size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Dec 2, 2024
@cdransf cdransf self-assigned this Dec 2, 2024
Copy link

changeset-bot bot commented Dec 2, 2024

🦋 Changeset detected

Latest commit: d53abe8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/colorslider Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Dec 2, 2024

🚀 Deployed on https://pr-3424--spectrum-css.netlify.app

Copy link
Contributor

github-actions bot commented Dec 2, 2024

File metrics

Summary

Total size: 1.73 MB*
Total change (Δ): 🔴 ⬆ 0.09 KB (0.00%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
colorslider 4.00 KB 🔴 ⬆ 0.09 KB

Details

colorslider

Filename Head Compared to base
index.css 4.00 KB 🔴 ⬆ 0.09 KB (2.24%)
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@cdransf cdransf force-pushed the cdransf/s2-colorslider-migration branch from b516928 to 97aad9d Compare December 3, 2024 23:31
@castastrophe castastrophe force-pushed the spectrum-two branch 3 times, most recently from cdb180d to 27d01df Compare December 4, 2024 14:54
@cdransf cdransf force-pushed the cdransf/s2-colorslider-migration branch 2 times, most recently from ea0cb43 to 295161b Compare December 10, 2024 15:18
@cdransf cdransf marked this pull request as ready for review December 17, 2024 14:20
@cdransf cdransf force-pushed the cdransf/s2-colorslider-migration branch from 295161b to ff4309f Compare December 17, 2024 14:23
@cdransf cdransf added ready-for-review and removed blocked See description and comments for what is blocking this issue labels Dec 17, 2024
@cdransf
Copy link
Member Author

cdransf commented Dec 17, 2024

Per design we can ignore the spacing tokens as they're not required for the implementation (alignment of the handle within the color slide is handled by this rule:

.spectrum-ColorSlider-handle {
  inset-inline-start: 0;
  inset-block-start: 50%;
}

@cdransf cdransf force-pushed the cdransf/s2-colorslider-migration branch from ff4309f to 37a6f2c Compare December 18, 2024 22:54
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Migration-wise, this looks fine, since not a lot needed to change here! I have a few other recommendations though:

CSS
I have a few recommendations for the CSS that would make this file more consistent with some of the other migration work that's been done:

  • Generally, we've been moving forced-colors media query blocks to the bottom in recent migrations (see feat(colorarea): S2 migration #3388, feat(tooltip): S2 migration #2743, or feat(statuslight): s2 migration #2818 for instance), and I see that Color Slider's is still further up in the CSS file, that could probably be moved down.
  • We have this line:
    block-size: var(--mod-color-slider-control-track-width, var(--spectrum-color-control-track-width));
    I'm really not sure why that color-control-track-width token has that name, but it seems like a good idea to create an alias custom property for this, especially given that its token name doesn't match its mod name, something like this, we can even take the word "control" out and change the mod name completely, up to you:
     /* Up at the topmost .spectrum-ColorSlider block */
     --spectrum-color-slider-control-track-width: var(--spectrum-color-control-track-width);
    
    /* further down in the code, same place it is now */
    block-size: var(--mod-color-slider-control-track-width, var(--spectrum-color-slider-control-track-width));
  • In that same vein, we could also alias the global token for the disabled background color that we have here, similar to how it's done in Color Wheel, Picker, or Search, for instance:
    --spectrum-color-slider-border-color-local: var(--highcontrast-color-slider-border-color-disabled, var(--mod-color-slider-border-color-disabled, var(--spectrum-disabled-background-color)));
    background: var(--highcontrast-color-slider-background-color-disabled, var(--mod-color-slider-background-color-disabled, var(--spectrum-disabled-background-color)));
  • There's also an unused token for the spacing between the field label and the color slider, but this isn't new. I don't know if there's some specific reason we're not adding the label to this implementation (Slider has a label that we display), but it feels like something we ought to be doing. Feel free to get more opinions on this one.
image

Storybook
Otherwise, this component could probably benefit from a few Storybook enhancements similar to what we added for several of our other components back when we were doing docs to storybook migrations (this component got migrated early on, before we decided that we would be doing that kind of thing):

  • Being able to change the colors in the slider from the Storybook controls or add an image (Swatch on main added this with the docs to storybook migration docs(swatch,swatchgroup,table,tabs): docs migrations to storybook #2925, so we might be able to copy that over)
  • A control to be able to view the vertical variant from the default story in Storybook. In this branch, all the variants are showing, but on main, on the default variant is visible and the others are used just for display purposes on the Docs page, that's probably where we'd like to end up one day when spectrum-two has some of those docs changes from main.

Last thing! I can't use the keyboard to focus on the slider handle (I can in main, but not on this branch or spectrum-two), this may not be a big deal, but I'm curious about why it's happening.

@cdransf cdransf force-pushed the cdransf/s2-colorslider-migration branch 2 times, most recently from b68e2a2 to d101918 Compare December 19, 2024 23:07
@cdransf cdransf changed the title feat(colorslider): s2 migration feat(colorslider): S2 migration Dec 19, 2024
@castastrophe castastrophe force-pushed the spectrum-two branch 5 times, most recently from e6d16bd to fc916f1 Compare December 29, 2024 19:21
@cdransf cdransf force-pushed the cdransf/s2-colorslider-migration branch 4 times, most recently from f9af38a to c6a5402 Compare January 6, 2025 16:00
@cdransf
Copy link
Member Author

cdransf commented Jan 6, 2025

Migration-wise, this looks fine, since not a lot needed to change here! I have a few other recommendations though:

CSS I have a few recommendations for the CSS that would make this file more consistent with some of the other migration work that's been done:

Alrighty! forced-colors has been moved to the bottom of the file.

  • We have this line:

    block-size: var(--mod-color-slider-control-track-width, var(--spectrum-color-control-track-width));

    I'm really not sure why that color-control-track-width token has that name, but it seems like a good idea to create an alias custom property for this, especially given that its token name doesn't match its mod name, something like this, we can even take the word "control" out and change the mod name completely, up to you:

     /* Up at the topmost .spectrum-ColorSlider block */
     --spectrum-color-slider-control-track-width: var(--spectrum-color-control-track-width);
    
    /* further down in the code, same place it is now */
    block-size: var(--mod-color-slider-control-track-width, var(--spectrum-color-slider-control-track-width));

--spectrum-color-control-track-width has been aliased to --spectrum-color-slider-control-track-width.

  • In that same vein, we could also alias the global token for the disabled background color that we have here, similar to how it's done in Color Wheel, Picker, or Search, for instance:
    --spectrum-color-slider-border-color-local: var(--highcontrast-color-slider-border-color-disabled, var(--mod-color-slider-border-color-disabled, var(--spectrum-disabled-background-color)));
    background: var(--highcontrast-color-slider-background-color-disabled, var(--mod-color-slider-background-color-disabled, var(--spectrum-disabled-background-color)));

--spectrum-disabled-background-color has been aliased to --spectrum-colorslider-fill-color-disabled.

  • There's also an unused token for the spacing between the field label and the color slider, but this isn't new. I don't know if there's some specific reason we're not adding the label to this implementation (Slider has a label that we display), but it feels like something we ought to be doing. Feel free to get more opinions on this one.

@jawinn and I discussed the label visible in this component and (apparently) SWC doesn't have the label as part of the component and the consensus was that we didn't need to include it at this point.

Storybook Otherwise, this component could probably benefit from a few Storybook enhancements similar to what we added for several of our other components back when we were doing docs to storybook migrations (this component got migrated early on, before we decided that we would be doing that kind of thing):

  • Being able to change the colors in the slider from the Storybook controls or add an image (Swatch on main added this with the docs to storybook migration docs(swatch,swatchgroup,table,tabs): docs migrations to storybook #2925, so we might be able to copy that over)
  • A control to be able to view the vertical variant from the default story in Storybook. In this branch, all the variants are showing, but on main, on the default variant is visible and the others are used just for display purposes on the Docs page, that's probably where we'd like to end up one day when spectrum-two has some of those docs changes from main.

I added a colors control to the story for the component that defaults to the previously hardcoded linear gradient and a vertical boolean control that defaults to false and allows the orientation to be changed.

Last thing! I can't use the keyboard to focus on the slider handle (I can in main, but not on this branch or spectrum-two), this may not be a big deal, but I'm curious about why it's happening.

I can reproduce this issue too, but haven't been able to sort out why it's happening yet. 🤔

@cdransf cdransf force-pushed the cdransf/s2-colorslider-migration branch from c6a5402 to afac5f7 Compare January 6, 2025 17:10
@cdransf cdransf requested a review from rise-erpelding January 6, 2025 17:13
@cdransf
Copy link
Member Author

cdransf commented Jan 6, 2025

I am able to focus on the handle in this branch (testing in Chrome) — I can't in Safari, but I often get odd keyboard focus behavior in Safari.

image

@cdransf cdransf force-pushed the cdransf/s2-colorslider-migration branch from afac5f7 to d53abe8 Compare January 6, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants