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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Oct 1, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
097f815
fix(slider ramp): correct handle and ramp vertical alignment
cdransf Sep 23, 2024
a8c11f4
fix(slider ramp): correct handle and ramp vertical alignment
cdransf Sep 24, 2024
6ccc2a4
fix(slider ramp): restore ramp margin
cdransf Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/rude-rivers-crash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@spectrum-css/slider": minor
---

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.
11 changes: 10 additions & 1 deletion components/slider/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@
min-inline-size: var(--mod-slider-min-size, var(--spectrum-slider-min-size));

user-select: none;

&:not(.spectrum-Slider--sideLabel) .spectrum-Slider-labelContainer + .spectrum-Slider-controls:has(.spectrum-Slider-ramp) {
margin-block-start: calc(var(--mod-slider-ramp-track-height, var(--spectrum-slider-ramp-track-height)) / 2);
}
}

.spectrum-Slider--sideLabel {
Expand Down Expand Up @@ -152,6 +156,11 @@
position: relative;
z-index: auto;

&:not(:has(.spectrum-Slider-ticks)) {
display: flex;
align-items: center;
}

/* These calculations prevent the track from spilling outside of the control */
inline-size: calc(100% - calc(var(--mod-slider-controls-margin, var(--spectrum-slider-controls-margin))) * 2);
margin-inline-start: var(--mod-slider-controls-margin, var(--spectrum-slider-controls-margin));
Expand Down Expand Up @@ -284,7 +293,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 •
edited
Loading

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.

block-size: var(--mod-slider-ramp-track-height, var(--spectrum-slider-ramp-track-height));

position: absolute;
Expand Down Expand Up @@ -412,6 +420,7 @@
&:lang(ko) {
line-height: var(--mod-slider-cjk-line-height, var(--spectrum-slider-cjk-line-height));
}

}

.spectrum-Slider-label {
Expand Down
2 changes: 2 additions & 0 deletions components/slider/metadata/metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
".spectrum-Slider--tick .spectrum-Slider-handle",
".spectrum-Slider--tick .spectrum-Slider-tickLabel",
".spectrum-Slider-controls",
".spectrum-Slider-controls:not(:has(.spectrum-Slider-ticks))",
".spectrum-Slider-fill",
".spectrum-Slider-fill--right",
".spectrum-Slider-fill:before",
Expand Down Expand Up @@ -89,6 +90,7 @@
".spectrum-Slider:not(.is-disabled, .spectrum-Slider--filled, .spectrum-Slider--range) .spectrum-Slider-controls:focus-within",
".spectrum-Slider:not(.is-disabled, .spectrum-Slider--filled, .spectrum-Slider--range) .spectrum-Slider-controls:hover",
".spectrum-Slider:not(.is-disabled, .spectrum-Slider--filled, .spectrum-Slider--range).js-focus-within .spectrum-Slider-controls[focus-within]",
".spectrum-Slider:not(.spectrum-Slider--sideLabel) .spectrum-Slider-labelContainer + .spectrum-Slider-controls:has(.spectrum-Slider-ramp)",
"[dir=\"rtl\"] .spectrum-Slider",
"[dir=\"rtl\"] .spectrum-Slider .spectrum-Slider-handle:before"
],
Expand Down
Loading