feat(slider): Implement vertical orientation #11028
Conversation
github-actions
bot
added
the
enhancement
…play of the long labels for horizontal and vertical
…hub.com:Esri/calcite-design-system into josercarcamo/5522-slider-vertical-horizontal-v4
jcfranco
added
the
pr ready for visual snapshots
|
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 |
|
It looks like your |
|
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
bot
added
the
Stale
github-actions
bot
removed
the
Stale
There was a problem hiding this comment.
Thanks, @josercarcamo! A few changes are needed before we can merge this.
| @@ -56,6 +56,10 @@ | |||
| :host { | |||
| @apply block; | |||
|
|
|||
| &:host([layout="vertical"]) { | |||
There was a problem hiding this comment.
The parent selector here is redundant. Let’s move it outside this block to simplify. Otherwise, it results in: :host:host([layout="vertical"]).
| @@ -56,6 +56,10 @@ | |||
| :host { | |||
| @apply block; | |||
|
|
|||
| &:host([layout="vertical"]) { | |||
| transform: rotate(-90deg); | |||
There was a problem hiding this comment.
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>
<span>top</span>
<calcite-slider
layout="vertical"
min-value="25"
max-value="75"
></calcite-slider>
<calcite-slider
min-value="25"
max-value="75"
></calcite-slider>
<span>bottom</span>
</body>There was a problem hiding this comment.
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.
| @@ -138,6 +142,7 @@ | |||
| content: "\2014"; | |||
| display: inline-block; | |||
| inline-size: 1em; | |||
| margin-inline-start: 0.1875rem; | |||
There was a problem hiding this comment.
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
| <span | ||
| aria-hidden="true" | ||
| class={{ | ||
| [thumbLabelClasses]: true, |
There was a problem hiding this comment.
Can you move the classes from the template string into the class object?
There was a problem hiding this comment.
Do you mean to move the classes here?
const thumbLabelClasses = ${CSS.handleLabel} ${ isMinThumb ? CSS.handleLabelMinValue : CSS.handleLabelValue };
To here?
const labels = isLabeled
? [
<span
aria-hidden="true"
class={{
[thumbLabelClasses]: true,
}}
>
{displayedValue}
,
<span ariaHidden="true" class={${thumbLabelClasses} ${CSS.static}}>
{displayedValue}
,
<span ariaHidden="true" class={${thumbLabelClasses} ${CSS.transformed}}>
{displayedValue}
,
]
: [];
| @@ -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 */ | |||
There was a problem hiding this comment.
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
| 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>`, |
There was a problem hiding this comment.
This should be using height, right?
| 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>`, |
There was a problem hiding this comment.
margin-top shouldn't be necessary here. This seems related to the layout issue mentioned earlier.
|
|
||
| const [trackX, trackY] = await getElementXY(page, "calcite-slider", ".track"); | ||
| expect(await slider.getProperty("value")).toBe(5); | ||
| expect(inputEvent).toHaveReceivedEventTimes(layout === "horizontal" ? 5 : 6); |
There was a problem hiding this comment.
Could you look into this? Changing layout shouldn't affect event behavior.
|
|
||
| expect(isMaxThumbFocused).toBe(true); | ||
| await assertValuesUnchanged(5); | ||
| layout === "horizontal" |
There was a problem hiding this comment.
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.
| max="1" | ||
| min-value="0" | ||
| max-value="0" | ||
| layout={layout}`; |
There was a problem hiding this comment.
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
Related Issue: #5522
Summary
Implemented the vertical orientation for slider.