-
Notifications
You must be signed in to change notification settings - Fork 194
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 52e9356 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
🚀 Deployed on https://pr-3154--spectrum-css.netlify.app |
423d759
to
4d4e7d8
Compare
File metricsSummaryTotal size: 4.31 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsslider
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
4d4e7d8
to
a902a7a
Compare
5d7c489
to
be9fd5f
Compare
eeaf57c
to
5855370
Compare
5855370
to
c56ec66
Compare
@@ -284,7 +289,6 @@ | |||
} | |||
|
|||
.spectrum-Slider-ramp { | |||
margin-block-start: calc(var(--mod-slider-ramp-track-height, var(--spectrum-slider-ramp-track-height)) / 2); |
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.
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
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.
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.
c56ec66
to
0fb8597
Compare
c182ba2
to
2c25f1a
Compare
2c25f1a
to
37f73fc
Compare
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.
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
Default
.Regression testing
Validate:
Screenshots
To-do list