feat(slider): Implement vertical orientation #11028

josercarcamo · 2024-12-11T18:34:24Z

Related Issue: #5522

Summary

Implemented the vertical orientation for slider.

…play of the long labels for horizontal and vertical

…hub.com:Esri/calcite-design-system into josercarcamo/5522-slider-vertical-horizontal-v4

benelan · 2024-12-12T02:06:23Z

What happened to #9953? Closing a PR with a lot of context in favor of a new one makes reviewing more difficult because we don't know what's been addressed and what needs further changes/discussion.

Can you please try maintain a single branch/PR for an issue's changes instead of doing this branch-v1, branch-v2, etc. style of iteration? Thanks!

benelan · 2024-12-12T02:10:11Z

It looks like your v3 PR (#10652) is still open too, which one should we be reviewing?

github-actions · 2024-12-26T01:57:35Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

jcfranco

Thanks, @josercarcamo! A few changes are needed before we can merge this.

jcfranco · 2025-01-06T18:04:13Z

packages/calcite-components/src/components/slider/slider.scss

@@ -56,6 +56,10 @@
 :host {
  @apply block;

+  &:host([layout="vertical"]) {


The parent selector here is redundant. Let’s move it outside this block to simplify. Otherwise, it results in: :host:host([layout="vertical"]).

jcfranco · 2025-01-06T18:22:24Z

packages/calcite-components/src/components/slider/slider.scss

@@ -56,6 +56,10 @@
 :host {
  @apply block;

+  &:host([layout="vertical"]) {
+    transform: rotate(-90deg);


I'm noticing some layout issues. It seems the vertical layout isn’t respecting the explicitly set height on the component.

<body> <style> calcite-slider { border: 1px solid red; &[layout="horizontal"] { width: 100px; } &[layout="vertical"] { height: 100px; } } </style> top <calcite-slider layout="vertical" min-value="25" max-value="75" ></calcite-slider> <calcite-slider min-value="25" max-value="75" ></calcite-slider> bottom </body>

Although the content box has been rotated via the rule:

:host([layout="vertical"]) { transform: rotate(-90deg); }

CSS interprets the properties as if the scroll bar had not been rotated, so "width" controls the height and "height" controls the width when vertical. Please advise if you know of a way to change the "orientation" of the properties as well. I'm thinking we set a watcher on "width" and "height" and set the properties correctly when vertical.

I'm not sure I follow the proposed approach, but let's discuss this further today.

jcfranco · 2025-01-06T18:48:18Z

packages/calcite-components/src/components/slider/slider.scss

@@ -138,6 +142,7 @@
      content: "\2014";
      display: inline-block;
      inline-size: 1em;
+      margin-inline-start: 0.1875rem;


Nice! Is there a token we could reference instead of hardcoding it? We have the following ones available: --calcite-spacing-xxs (4px), --calcite-spacing-base (2px). cc @ashetland

jcfranco · 2025-01-06T18:51:13Z

packages/calcite-components/src/components/slider/slider.tsx

+          <span
+            aria-hidden="true"
+            class={{
+              [thumbLabelClasses]: true,


Can you move the classes from the template string into the class object?

Do you mean to move the classes here?

const thumbLabelClasses = ${CSS.handleLabel} ${ isMinThumb ? CSS.handleLabelMinValue : CSS.handleLabelValue };

To here?

const labels = isLabeled
? [

{displayedValue}
,

{displayedValue}
,

{displayedValue}
,
]
: [];

I meant something like this:

const labelClasses = { [CSS.handleLabel]: true, [CSS.handleLabelMinValue]: isMinThumb [CSS.handleLabelValue]: isMinThumb }; // ... {displayedValue} , {displayedValue} , {displayedValue} ```

jcfranco · 2025-01-06T19:00:00Z

packages/calcite-components/src/components/slider/slider.tsx

@@ -320,6 +322,9 @@ export class Slider
  /** The component's value. */
  @property({ type: Number, reflect: true }) value: null | number | number[] = 0;

+  /** The orientation of the slider */


We avoid using the actual name of components in the doc. Can you change this to follow existing props (e.g., segmented-conttrol#layout)? cc @geospatialem @DitwanP

jcfranco · 2025-01-06T20:05:38Z

packages/calcite-components/src/components/slider/slider.e2e.ts

+    describe("mouse interaction", () => {
+      it("single handle: clicking the track changes value on mousedown, emits on mouseup", async () => {
+        const page = await newE2EPage({
+          html: `<calcite-slider snap style="width:${sliderWidthFor1To1PixelValueTrack}" layout=${layout}></calcite-slider>`,


This should be using height, right?

No. "width = height" in vertical orientation.

This is confusing, possibly because the implementation is relying heavily on the transform. From a user-perspective, width and height should not change semantics based on an option.

Let's discuss this further today.

jcfranco · 2025-01-06T20:06:39Z

packages/calcite-components/src/components/slider/slider.e2e.ts

-      expect(await slider.getProperty("maxValue")).toBe(75);
+      it("single handle: clicking and dragging the track changes and emits the value", async () => {
+        const page = await newE2EPage({
+          html: `<calcite-slider snap style="width:${sliderWidthFor1To1PixelValueTrack}; margin-top: 100px" layout=${layout}></calcite-slider>`,


margin-top shouldn't be necessary here. This seems related to the layout issue mentioned earlier.

It is for aesthetics.

E2E tests aren't visual by default, so aesthetics aren't a concern in this context.

I guess I misspoke: a part of the vertical slider could be outside the bounds of the window and not be clickable, so it must be moved downward.

jcfranco · 2025-01-06T20:07:40Z

packages/calcite-components/src/components/slider/slider.e2e.ts


-      const [trackX, trackY] = await getElementXY(page, "calcite-slider", ".track");
+        expect(await slider.getProperty("value")).toBe(5);
+        expect(inputEvent).toHaveReceivedEventTimes(layout === "horizontal" ? 5 : 6);


Could you look into this? Changing layout shouldn't affect event behavior.

The problem is that the upper-left hand corner given by "getElementXY" for horizontal is at value 0 of the slider, but it is at 100 for vertical. This causes one extra event:

Oh, I see. In that case, the starting position for this test for the vertical case should be adjusted to start at the same value.

jcfranco · 2025-01-06T20:10:05Z

packages/calcite-components/src/components/slider/slider.e2e.ts


-        expect(isMaxThumbFocused).toBe(true);
-        await assertValuesUnchanged(5);
+          layout === "horizontal"


Can you DRY this up by storing the test x, y variables based on layout and passing them to click? Applies to similar test steps.

jcfranco · 2025-01-06T20:17:04Z

packages/calcite-components/src/components/slider/slider.e2e.ts

+        max="1"
+        min-value="0"
+        max-value="0"
+        layout={layout}`;


Shouldn't this be layout=${layout} to use the layout var from the describe.each block? This would eliminate the need to use replace below and keeps tests consistent.

…ved mention of slider in comment; moved definition of shadowRoot up

josercarcamo · 2025-01-17T00:07:26Z

After meeting with @jcfranco, we've decided to seek an alternative implementation and pause this issue at this time.

feat(slider): Implement vertical orientation

bb2a127

github-actions bot assigned josercarcamo Dec 11, 2024

github-actions bot added the enhancement Issues tied to a new feature or request. label Dec 11, 2024

josercarcamo and others added 3 commits December 11, 2024 10:35

Merge branch 'dev' into josercarcamo/5522-slider-vertical-horizontal-v4

ff6ff48

Fixed the separation between the vertical sliders; also fixed the dis…

a93e233

…play of the long labels for horizontal and vertical

Merge branch 'josercarcamo/5522-slider-vertical-horizontal-v4' of git…

20719e1

…hub.com:Esri/calcite-design-system into josercarcamo/5522-slider-vertical-horizontal-v4

jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Dec 11, 2024

Improved display of dash separating handle values

9eaec7b

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Dec 26, 2024

josercarcamo mentioned this pull request Jan 2, 2025

feat(slider): Implement vertical orientation #10652

Closed

Merge branch 'dev' into josercarcamo/5522-slider-vertical-horizontal-v4

1ef1b92

github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Jan 3, 2025

jcfranco requested changes Jan 6, 2025

View reviewed changes

jcfranco requested a review from ashetland January 6, 2025 20:24

josercarcamo added 7 commits January 13, 2025 17:32

Moved host rule per code review

27ff973

Merge branch 'dev' into josercarcamo/5522-slider-vertical-horizontal-v4

e4bd87b

Fixed spacing around hyphen of range label; reused getTextWidth; remo…

ba3ee46

…ved mention of slider in comment; moved definition of shadowRoot up

Changed how the shorthand font is obtained

0f0c3fd

Changed name of variable

cf77344

Changed name of 'pixel' to 'value':

9eb46f6

Removed style to preview-head.html

46f4fe7

josercarcamo requested a review from benelan as a code owner January 15, 2025 23:14

josercarcamo and others added 2 commits January 16, 2025 11:38

Merge branch 'dev' into josercarcamo/5522-slider-vertical-horizontal-v4

7bba4a3

test: fix linting errors

7a08c49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(slider): Implement vertical orientation #11028

feat(slider): Implement vertical orientation #11028

josercarcamo commented Dec 11, 2024

benelan commented Dec 12, 2024

benelan commented Dec 12, 2024

github-actions bot commented Dec 26, 2024

jcfranco left a comment •

edited

Loading

jcfranco Jan 6, 2025

jcfranco Jan 6, 2025

josercarcamo Jan 14, 2025 •

edited

Loading

jcfranco Jan 16, 2025

jcfranco Jan 6, 2025

jcfranco Jan 6, 2025

josercarcamo Jan 14, 2025 •

edited

Loading

jcfranco Jan 16, 2025

jcfranco Jan 6, 2025

jcfranco Jan 6, 2025

josercarcamo Jan 15, 2025

jcfranco Jan 16, 2025

jcfranco Jan 6, 2025

josercarcamo Jan 15, 2025

jcfranco Jan 16, 2025

josercarcamo Jan 16, 2025

jcfranco Jan 6, 2025 •

edited

Loading

josercarcamo Jan 16, 2025

jcfranco Jan 16, 2025

jcfranco Jan 6, 2025

jcfranco Jan 6, 2025

josercarcamo commented Jan 17, 2025

feat(slider): Implement vertical orientation #11028

Are you sure you want to change the base?

feat(slider): Implement vertical orientation #11028

Conversation

josercarcamo commented Dec 11, 2024

Summary

benelan commented Dec 12, 2024

benelan commented Dec 12, 2024

github-actions bot commented Dec 26, 2024

jcfranco left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josercarcamo Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josercarcamo Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josercarcamo commented Jan 17, 2025

jcfranco left a comment •

edited

Loading

josercarcamo Jan 14, 2025 •

edited

Loading

josercarcamo Jan 14, 2025 •

edited

Loading

jcfranco Jan 6, 2025 •

edited

Loading