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

fix(slider ramp): correct handle and ramp vertical alignment #3154

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Sep 23, 2024

Description

This applies a flex layout to the spectrum slider controls to consistently align the enclosed handle and ramp, while also removing the margins that might otherwise interfere with alignment. This aims to resolve the issues with the calc-based approach that exhibited variations in vertical alignment.

How and where has this been tested?

Checked locally in Storybook.

Validation steps

  1. Pull down the branch and run Storybook locally or access the Storybook URL for the PR.
  2. Open the Slider component and select Default.
  3. Verify that the handle is vertically aligned over the track.

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

Screenshot 2024-09-30 at 3 56 14 PM

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. ✨

Copy link

changeset-bot bot commented Sep 23, 2024

🦋 Changeset detected

Latest commit: 52e9356

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/slider 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

@cdransf cdransf added the run_vrt For use on PRs looking to kick off VRT label Sep 23, 2024
Copy link
Contributor

github-actions bot commented Sep 23, 2024

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

@cdransf cdransf force-pushed the cdransf/slider-ramp-control-fix branch from 423d759 to 4d4e7d8 Compare September 23, 2024 19:42
@cdransf cdransf marked this pull request as ready for review September 23, 2024 19:44
Copy link
Contributor

github-actions bot commented Sep 23, 2024

File metrics

Summary

Total size: 4.31 MB*
Total change (Δ): 🔴 ⬆ 0.99 KB (0.02%)

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

Package Size Δ
slider 32.49 KB 🔴 ⬆ 0.33 KB

Details

slider

Filename Head Compared to base
index-base.css 30.18 KB 🔴 ⬆ 0.33 KB (1.08%)
index-theme.css 2.92 KB -
index-vars.css 32.49 KB 🔴 ⬆ 0.33 KB (1.00%)
index.css 32.49 KB 🔴 ⬆ 0.33 KB (1.00%)
themes/express.css 1.78 KB -
themes/spectrum.css 1.75 KB -
* 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 marked this pull request as draft September 23, 2024 19:59
@cdransf cdransf force-pushed the cdransf/slider-ramp-control-fix branch from 4d4e7d8 to a902a7a Compare September 23, 2024 20:25
@cdransf cdransf marked this pull request as ready for review September 23, 2024 20:25
@cdransf cdransf force-pushed the cdransf/slider-ramp-control-fix branch 3 times, most recently from 5d7c489 to be9fd5f Compare September 24, 2024 14:51
@cdransf cdransf force-pushed the cdransf/slider-ramp-control-fix branch from eeaf57c to 5855370 Compare September 26, 2024 00:54
@cdransf cdransf closed this Sep 26, 2024
@cdransf cdransf reopened this Sep 26, 2024
@cdransf cdransf force-pushed the cdransf/slider-ramp-control-fix branch from 5855370 to c56ec66 Compare September 26, 2024 23:11
@@ -284,7 +289,6 @@
}

.spectrum-Slider-ramp {
margin-block-start: calc(var(--mod-slider-ramp-track-height, var(--spectrum-slider-ramp-track-height)) / 2);
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt Sep 30, 2024

Choose a reason for hiding this comment

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

Do we need to preserve this spacing between the ramp container and the labels? Without it, both containers touch. To me, it looks like this spacing will need to be defined in S2 as well (just based on the "spacing (field label to slider control" and the "spacing (top/bottom edge to label)" headings). Maybe instead of removing it completely, we can move it elsewhere? Into a different selector block? I don't even know if that's possible, you probably know better than me at this point.

Screen.Recording.2024-09-30.at.9.29.45.AM.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

Alrighty! I've added the following to the root .spectrum-Slider class:

&:not(.spectrum-Slider--sideLabel) .spectrum-Slider-labelContainer + .spectrum-Slider-controls:has(.spectrum-Slider-ramp) {
  margin-block-start: var(--mod-spectrum-slider-value-side-padding-block, var(--spectrum-slider-value-side-padding-block));
}

Adding the 8px gap called for in the Figma for S2 to the .spectrum-Slider-controls provided the component is not in the side label configuration but is using the ramp design. I'm wondering if that selector is too verbose or perhaps too fragile, but it does need to balance all of those different cases and preserve the spacing without being applied to the .spectrum-Slider-ramp class directly as that does cause the misalignment with the slider control to crop back up.

@cdransf cdransf force-pushed the cdransf/slider-ramp-control-fix branch from c56ec66 to 0fb8597 Compare September 30, 2024 15:38
@cdransf cdransf force-pushed the cdransf/slider-ramp-control-fix branch 2 times, most recently from c182ba2 to 2c25f1a Compare September 30, 2024 16:09
@cdransf cdransf force-pushed the cdransf/slider-ramp-control-fix branch from 2c25f1a to 37f73fc Compare September 30, 2024 20:12
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.

Looked at all sizes, small and XL particularly are a great improvement!
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review run_vrt For use on PRs looking to kick off VRT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants