feat(slider): Implement vertical orientation #9953

josercarcamo · 2024-08-02T19:34:10Z

Related Issue: #5522

Summary

Implemented a vertical orientation for the slider, along with label, precise, ticks.

github-actions · 2024-09-15T02:00:59Z

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.

github-actions · 2024-10-12T01:56:37Z

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.

benelan · 2024-10-22T06:36:42Z

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

-    expect(button).toEqualAttribute("aria-valuemin", "0");
-    expect(button).toEqualAttribute("aria-valuemax", "100");
-    expect(button).toEqualAttribute("aria-orientation", "horizontal");
+    for (const l of ["horizontal", "vertical"]) {


Instead of adding a for loop inside the tests, can you wrap them all in a describe.each block. Something like this:

describe.each(["horizontal", "vertical"])("%s layout", (layout) => { // tests here })

NOTE: I typed that directly into the comment so the syntax may be off.

This will make it easier to determine which layout is causing issues when there are test failures. Also we have a convention not to abbreviate so I changed l to layout.

benelan · 2024-10-22T06:39:59Z

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

-    scale="m"
-  ></calcite-slider>
-`;
+const simple = {};


We don't need separate "simple" stories for the layout property because it can be changed via the knobs.

benelan · 2024-10-22T06:43:20Z

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

+  };
+
+const rangeLabeledTicksEdgePositioningAtMin = {};
+for (const l of ["horizontal", "vertical"]) {


I don't think all of these stories need to be duplicated for vertical vs horizontal.

How about one vertical story that has a couple sliders with different configurations that might be impacted by the change in layout?

jcfranco

Great progress, @josercarcamo! 💪

I brought up some things that need to be addressed. LMK if you have any questions.

jcfranco · 2024-10-21T23:11:19Z

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

@@ -559,8 +576,9 @@ export class Slider
      return;
    }

-    const x = event.clientX || event.pageX;
-    const position = this.mapToRange(x);
+    const v =


What does v represent? If it's value, can you rename it to be more expressive?

jcfranco · 2024-10-21T23:11:57Z

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

@@ -938,13 +958,20 @@ export class Slider
  /**
   * Translate a pixel position to value along the range
   *
-   * @param x
+   * @param p


Can you rename this to be expressive?

jcfranco · 2024-10-21T23:12:13Z

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

-    const { left, width } = this.trackEl.getBoundingClientRect();
-    const percent = (x - left) / width;
+    let percent: number;
+    if (this.layout === "horizontal") {


There's a bit of duplication here. Can you DRY it up?

jcfranco · 2024-10-21T23:29:49Z

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

@@ -247,6 +249,9 @@ export class Slider
  }

  componentDidRender(): void {
+    if (this.layout === "vertical") {


FIXME: this is bypassing logic that should apply to both layouts (e.g., avoiding label overlap). You can see the difference by comparing edge/overlapping stories in both layouts.

Actually, most of this code translates the handle label in the x direction, which is not needed for vertical.

This needs to handle vertical label overlap for both layouts. You can see this if you compare the edge/overlapping stories:

Range Labeled Ticks Edge Positioning At Max

Range Labeled Ticks Overlapping At Min

@ashetland Do we need hyphenation when vertical?

Nice catch! Yep, looks like we need this on vertical as well. While we're at it, is it possible to add a space so it would look like 99.5 - 100 instead of 99.5- 100?

Could we please update the design since the numbers are now one on top of the other? Or should it just display "99.5 - 100 |" where "|" is the vertical range bar whenever they overlap each other?

Yep, when the range handles overlap, it should display "99.5 - 100" like the horizontal layout.

jcfranco · 2024-10-21T23:30:43Z

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

+    ></calcite-slider>
+  `;
+}
+export const simpleH = simple["horizontal"];


For readability, avoid abbreviations in test names.

jcfranco · 2024-10-21T23:31:06Z

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

+export const rangeH = range["horizontal"];
+export const rangeV = range["vertical"];
+
+const darkMode = {};


Can you type this, and similar objects? May not be applicable if story functions are parameterized.

jcfranco · 2024-10-21T23:36:58Z

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

+    ></calcite-slider>
+  `;
+}
+export const simpleH = simple["horizontal"];


FIXME: can you restore the names for the previous, horizontal stories? Renamed stories will get treated as new ones and won't display a diff.

For improved organization, we could look at having separate stories for each slider layout configuration. I think we can use title for this separation and have Storybook take care of the hierarchy (e.g., title: "Components/Controls/Slider [default], title: "Components/Controls/Slider/Vertical" [new]). I'm open to other ideas too.

what if we make it so each story has two sliders - one horizontal and one vertical? That way we don't double slider's chromatic snapshots. Specifically for simple, we only need one slider because it can be changed via knobs like I mentioned above.

I dig this. Besides reducing snapshots, it also makes it easier to add coverage for both compared to handling them separately.

jcfranco · 2024-10-21T23:48:59Z

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

-`;
+const simple = {};
+for (const l of ["horizontal", "vertical"]) {
+  simple[l] = (args: SliderStoryArgs): string => html`


Storybook allows us to pass arguments to stories, so can you update these to use args:

const SimpleTemplate = { render: (args: SliderStoryArgs): string => html` <calcite-slider min="${args.min}" max="${args.max}" value="${args.value}" step="${args.step}" min-label="${args.minLabel}" ${boolean("disabled", args.disabled)} ${boolean("label-handles", args.labelHandles)} ${boolean("label-ticks", args.labelTicks)} ticks="${args.ticks}" page-step="${args.pageStep}" ${boolean("precise", args.precise)} ${boolean("mirrored", args.mirrored)} ${boolean("snap", args.snap)} scale="${args.scale}" layout=${args.layout || "horizontal"} style="${args.style}" ></calcite-slider> `; } export const simple = { ...SimpleTemplate, // default "horizontal" specified at [CSF (story metadata) level](https://github.com/Esri/calcite-design-system/pull/9953/files#diff-a4b7803179119e68983d2abf8a806a8e0be7c72550eee9a97a583b289c01ba0aR46) }; export const simpleVertical = { ...SimpleTemplate, args: { layout: "vertical" } };

See https://storybook.js.org/docs/writing-stories/stories-for-multiple-components#creating-a-template-component

Generalizing the template might help simplify setup for other stories.

jcfranco · 2024-10-21T23:51:56Z

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

@@ -42,6 +43,8 @@ export default {
    mirrored: false,
    snap: true,
    scale: scale.defaultValue,
+    layout: "horizontal",
+    style: "padding-top: 200px; padding-left: 100px; height: 200px",


FIXME: it looks like style was added to work around the slider not having the proper box model. In general, we only want to provide defaults for component props.

jcfranco · 2024-10-23T18:37:03Z

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

+          (this.layout === "horizontal" &&
+            (minThumbTypes.includes("histogram") || minThumbTypes.includes("precise"))) ||
+          (this.layout === "vertical" && valueIsRange && this.precise && !this.mirrored) ||
+          (this.layout === "vertical" && valueIsRange && this.precise && this.mirrored)


mirrored is canceled out in these or statements, so you could simplify this to:

thumbPlacement: (this.layout === "horizontal" && (minThumbTypes.includes("histogram") || minThumbTypes.includes("precise"))) || (this.layout === "vertical" && valueIsRange && this.precise);

github-actions bot assigned josercarcamo Aug 2, 2024

josercarcamo changed the title ~~[DRAFT] feat(slider): Implement vertical orientation~~ feat(slider): Implement vertical orientation Aug 2, 2024

josercarcamo marked this pull request as draft August 2, 2024 19:49

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

github-actions bot added Stale Issues or pull requests that have not had recent activity. and removed Stale Issues or pull requests that have not had recent activity. labels Sep 15, 2024

josercarcamo added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Sep 18, 2024

josercarcamo added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 27, 2024

josercarcamo added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 1, 2024

josercarcamo marked this pull request as ready for review October 2, 2024 19:39

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

github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Oct 22, 2024

benelan reviewed Oct 22, 2024

View reviewed changes

jcfranco requested changes Oct 23, 2024

View reviewed changes

josercarcamo added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 25, 2024

josercarcamo closed this Oct 28, 2024

josercarcamo force-pushed the josercarcamo/5522-slider-vertical-horizontal branch from db157ab to d7c9321 Compare October 28, 2024 18:03

josercarcamo mentioned this pull request Oct 30, 2024

feat(slider): Implement vertical orientation #10652

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(slider): Implement vertical orientation #9953

feat(slider): Implement vertical orientation #9953

josercarcamo commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Sep 15, 2024

github-actions bot commented Oct 12, 2024

benelan Oct 22, 2024 •

edited

Loading

benelan Oct 22, 2024

benelan Oct 22, 2024

jcfranco left a comment

jcfranco Oct 21, 2024

jcfranco Oct 21, 2024

jcfranco Oct 21, 2024

jcfranco Oct 21, 2024

josercarcamo Oct 24, 2024 •

edited

Loading

jcfranco Oct 24, 2024

ashetland Oct 24, 2024

josercarcamo Oct 25, 2024 •

edited

Loading

ashetland Oct 25, 2024

jcfranco Oct 21, 2024

jcfranco Oct 21, 2024

jcfranco Oct 21, 2024

benelan Oct 23, 2024 •

edited

Loading

jcfranco Oct 24, 2024

jcfranco Oct 21, 2024

jcfranco Oct 21, 2024

jcfranco Oct 23, 2024

feat(slider): Implement vertical orientation #9953

feat(slider): Implement vertical orientation #9953

Conversation

josercarcamo commented Aug 2, 2024 • edited Loading

Summary

github-actions bot commented Sep 15, 2024

github-actions bot commented Oct 12, 2024

benelan Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco left a 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

josercarcamo Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josercarcamo Oct 25, 2024 • 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

benelan Oct 23, 2024 • 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 Aug 2, 2024 •

edited

Loading

benelan Oct 22, 2024 •

edited

Loading

josercarcamo Oct 24, 2024 •

edited

Loading

josercarcamo Oct 25, 2024 •

edited

Loading

benelan Oct 23, 2024 •

edited

Loading