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.

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): added filled offset variant #3876

Merged
merged 28 commits into from Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3e2bc1e
chore(slider): added filled offset variant
Dec 15, 2023
c067b70
chore(slider): updated slider to take fill-start as a property
Dec 15, 2023
fafa81c
chore(slider): added check on starting point variable for truthy value
Dec 18, 2023
0023dec
Merge branch 'main' of https://github.com/adobe/spectrum-web-componen…
Dec 18, 2023
2e74cbb
chore(slider): updated golden image cache
Dec 18, 2023
cc80656
chore(slider): updated tests
Dec 18, 2023
81f3637
chore: resolved merge conflicts
Jan 2, 2024
752e8ef
chore(slider): updated golden image cache
Jan 2, 2024
f7b922d
chore: resolved merged conflicts
Jan 3, 2024
0558aad
chore(slider): slider with fill center and fill start value
Jan 6, 2024
b8192f0
chore(slider): updated README
Jan 6, 2024
7a4521d
chore(slider): updated golden image cache
Jan 6, 2024
4efe2e3
chore: resolved merge conflicts
Jan 10, 2024
5a2992d
chore(slider): updated golden image cache
Jan 10, 2024
524493d
chore(slider): code optimizations
Jan 11, 2024
e3176f2
chore(slider): updated fill on first load in willUpdate
Jan 11, 2024
09583f8
chore(slider): updated tests
Jan 11, 2024
e279f8a
chore(slider): updated filled offset slider storybook and docs
Jan 12, 2024
986bd3c
chore(slider): updated golden image cache
Jan 12, 2024
d73578e
Merge branch 'main' of https://github.com/adobe/spectrum-web-componen…
Jan 12, 2024
3b32275
chore(slider): updated logic for fill start with value
Jan 16, 2024
d203804
chore(slider): updated golden image cache
Jan 16, 2024
20cff87
Merge branch 'main' of https://github.com/adobe/spectrum-web-componen…
Jan 17, 2024
35b1816
chore(slider): code clean up
Jan 17, 2024
9af3468
chore(slider): updated tests
Jan 17, 2024
095fd7d
chore(slider): updated golden image cache
Jan 17, 2024
c9b4801
chore(slider): updated story
Jan 17, 2024
1b802da
chore(slider): updated golden image cache
Jan 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
Expand Up @@ -10,7 +10,7 @@ executors:
parameters:
current_golden_images_hash:
type: string
default: e8720ac5d0ee0d076e61309294041334afe2c160
default: 4efe2e33f04f30cd4965e4d8e252edc0f3b6cf76
wireit_cache_name:
type: string
default: wireit
Expand Down
79 changes: 78 additions & 1 deletion packages/slider/README.md
Expand Up @@ -83,14 +83,91 @@ import { Slider } from '@spectrum-web-components/slider';
### Filled

```html
<sp-slider label="Slider Label" variant="filled"></sp-slider>
<sp-slider
label="Slider Label"
max="1"
variant="filled"
min="0"
value=".5"
step="0.01"
></sp-slider>
<sp-slider
label="Slider Label - Disabled"
max="1"
variant="filled"
min="0"
value=".5"
step="0.01"
disabled
></sp-slider>
```

### Filled Offset with only fill-start

```html
<sp-slider
label="Slider Label"
max="1"
fill-start
min="0"
value=".5"
step="0.01"
></sp-slider>
<sp-slider
label="Slider Label"
max="1"
fill-start
min="0"
value=".5"
step="0.01"
disabled
></sp-slider>
```

### Filled Offset with fill-start value

```html-live
<sp-slider
id="fill-start-slider"
label="Slider Label"
max="1"
min="0"
value=".7"
step="0.1"
></sp-slider>
<sp-slider
label="Slider Label"
max="1"
min="0"
value=".7"
step="0.1"
disabled
></sp-slider>
<script type="module">
const initSlider = async () => {
const slider = document.querySelector('#fill-start-slider');
slider.fillStart = 0.3
};
customElements.whenDefined('code-example').then(() => {
customElements.whenDefined('sp-slider').then(() => {
initSlider();
});
});
</script>
```

<script type="module">
const initSlider = async () => {
const slider = document.querySelector('#fill-start-slider');
slider.fillStart = 0.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not apply this with the fill-start attribute?

Copy link
Contributor Author

@Rajdeepc Rajdeepc Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted the logic to add fill-start as an empty attribute as well as with a value

};
customElements.whenDefined('code-example').then(() => {
customElements.whenDefined('sp-slider').then(() => {
initSlider();
});
});
</script>

### Tick

```html
Expand Down
86 changes: 84 additions & 2 deletions packages/slider/src/Slider.ts
Expand Up @@ -14,6 +14,7 @@ import {
CSSResultArray,
html,
nothing,
PropertyValues,
SizedMixin,
TemplateResult,
} from '@spectrum-web-components/base';
Expand Down Expand Up @@ -91,6 +92,9 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), {
@property()
public type = '';

@property({ reflect: true })
public override dir!: 'ltr' | 'rtl';

@property({ type: String })
public set variant(variant: string) {
const oldVariant = this.variant;
Expand Down Expand Up @@ -160,6 +164,11 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), {
@property({ type: Boolean, reflect: true })
public override disabled = false;

@property({ type: Boolean, reflect: true, attribute: 'fill-start' })
public fillStart = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is supposed to be a boolean. I think it is supposed to represent where the fill starts from. By default it would equal the same as min but then set it would allow for the fill to start from whereever.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, maybe be unneeded work, but should we also surface a relativeValue property that give the value relative to the start? Calculable from the outside, so maybe not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should min the start value or the initial value represented as 'value' ?
fillStart boolean creates the filled highlight on the track.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be misunderstand, so let's check with the design team, but I understand the workflow to be:

<sp-slider></sp-slider> // No fill

<sp-slider
    variant="filled"
></sp-slider> // Fill from `min`, or 0 by default

<sp-slider
    variant="filled"
    fill-start="5"
    min="0"
    max="10"
></sp-slider> // Fill from 5, or half way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I was wondering if fill-start="5" and value="6" then how will the slider behave? which takes precendence?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0                    20
|----==0-------------|

I think it would look like this where the fill-started at 5 and the fill went to 6 where the handle stood. More clear might be variant="filled" fill-start value="15" min="0" max="20", variant="filled" fill-start="5" value="15" min="0" max="20", and variant="filled" fill-start="5" value="0" min="0" max="20" as comparison:

0                    20
|----------====0-----|
0                    20
|----==========0-----|
0                    20
0===-----------------|

When fill-start is present, the fill starts at half the value like you've previously outlined. When fill-start is supplied a value, the fill starts from that value. The fill goes from that number to the value provided. The delta from the fill-start to the value would be the relativeValue if we chose to surface it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. For this variant variant="filled" fill-start value="15" min="0" max="20" , handle will be at 10 i.e the center.
    When user slides from 10 - 20 and 10 - 0 should the fill be visible for both or it should only be visible from 10-15?

  2. For this variant variant="filled" fill-start="5" value="15" min="0" max="20"
    the handle should start from the fill-start value of "5" or from value="15" itself? And also do we show fill from 5-15 only or from 5-0 also?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variant="filled" fill-start value="15" min="0" max="20"
In this case the "fill" will start at half (so 10) but the value will be 15, so the "fill" will take up 1/4 of the track starting from the middle.

0                    20
|----------====0-----|

variant="filled" fill-start value="10" min="0" max="20"
Fill starting at half (10) and value at 10.

0                    20
|----------0---------|

User moves handle to 20

0                    20
|----------==========0

User moves handle to 0

0                    20
0===========---------|

variant="filled" fill-start="5" value="15" min="0" max="20"
Fill takes up half of the track starting 1/4 of the way along the track.

0                    20
|-----=========0-----|

User moves to 0, fill size is 1/4 of the track starting at the beginning of the track.

0                    20
0=====---------------|

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attached video recording for preview and review. Let me know how this solution looks now.


@property({ type: Number })
public fillStartPoint: number | undefined;
/**
* Applies `quiet` to the underlying `sp-number-field` when `editable === true`.
*/
Expand Down Expand Up @@ -348,6 +357,60 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), {
`;
}

private _cachedValue: number | undefined;

/**
* @description calculates the fill width
* @param fillStartValue
* @param currentValue
* @param cachedValue
* @returns
*/
private getOffsetWidth(
fillStartValue: number,
currentValue: number,
cachedValue: number
): number {
const distance =
fillStartValue === cachedValue
? Math.abs(currentValue - fillStartValue)
: Math.abs(
Math.min(currentValue, cachedValue) - fillStartValue
);

return (distance / (this.max - this.min)) * 100;
}

private getOffsetPosition(v: number): number {
const val = ((v - this.min) / (this.max - this.min)) * 100;
return val;
}

private renderFillOffset(): TemplateResult {
if (!this.fillStart || !this.fillStartPoint || !this._cachedValue) {
return html``;
}
return html`
<div
class=${`fill ${
!!(this.value > this.fillStartPoint) ? 'offset' : ''
}`}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using classMap({fill: true, offset: this.value > this.fillStartPoint}) might make this easier to read.

style=${styleMap({
[this.dir === 'rtl' ? 'right' : 'left']: `${
this.value > this.fillStartPoint
? this.getOffsetPosition(this.fillStartPoint)
: this.getOffsetPosition(this.value)
}%`,
width: `${this.getOffsetWidth(
this.fillStartPoint,
this.value,
this._cachedValue
)}%`,
})}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a complex map like this, we may want to create it as a variable outside of the template, so that it is easier to read.

Copy link
Contributor Author

@Rajdeepc Rajdeepc Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been updated to make it more modular

></div>
`;
}

private renderTrack(): TemplateResult {
const segments = this.handleController.trackSegments();
const handleItems = [
Expand All @@ -360,6 +423,7 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), {
id: `track${index + 1}`,
html: this.renderTrackSegment(start, end),
})),
{ id: 'fill', html: this.renderFillOffset() },
];

return html`
Expand Down Expand Up @@ -438,8 +502,12 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), {
const size = end - start;
const styles: StyleInfo = {
width: `${size * 100}%`,
'--spectrum-slider-track-background-size': `${(1 / size) * 100}%`,
'--spectrum-slider-track-segment-position': `${start * 100}%`,
...(this.handleController.size > 1 && {
'--spectrum-slider-track-background-size': `${
(1 / size) * 100
}%`,
'--spectrum-slider-track-segment-position': `${start * 100}%`,
}),
Rajdeepc marked this conversation as resolved.
Show resolved Hide resolved
};
return styles;
}
Expand All @@ -455,4 +523,18 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), {
await this.handleController.handleUpdatesComplete();
return complete;
}

protected override updated(changes: PropertyValues<this>): void {
super.updated(changes);
if (changes.has('value') && changes.has('fillStart')) {
this._cachedValue = Number(this.value);
if (typeof this.fillStart === 'number') {
this.fillStartPoint = this.fillStart;
} else {
this.fillStartPoint =
(Number(this.max) - Number(this.min)) / 2 +
Number(this.min);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this just be a getter instead? That would allow us to not place fillStartPoint as a reactive property, as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now using the fillStartPoint getter

private get fillStartPoint(): number {
        if (this.fillStart) {
            return Number(this.fillStart);
        } else {
            return (Number(this.max) - Number(this.min)) / 2 + Number(this.min);
        }
    }

}
}
3 changes: 2 additions & 1 deletion packages/slider/src/spectrum-config.js
Expand Up @@ -99,7 +99,6 @@ const config = {
},
converter.classToId('spectrum-Slider-buffer', 'buffer'),
converter.classToId('spectrum-Slider-controls', 'controls'),
converter.classToId('spectrum-Slider-fill', 'fill'),
converter.classToId('spectrum-Slider-label', 'label'),
converter.classToId(
'spectrum-Slider-labelContainer',
Expand All @@ -110,6 +109,8 @@ const config = {
converter.classToClass('spectrum-Slider-handle', 'handle'),
converter.classToClass('spectrum-Slider-input', 'input'),
converter.classToClass('spectrum-Slider-tick', 'tick'),
converter.classToClass('spectrum-Slider-fill--right', 'offset'),
converter.classToClass('spectrum-Slider-fill', 'fill'),
converter.classToClass(
'spectrum-Slider-tickLabel',
'tickLabel'
Expand Down
12 changes: 6 additions & 6 deletions packages/slider/src/spectrum-slider.css
Expand Up @@ -207,7 +207,7 @@ governing permissions and limitations under the License.
var(--spectrum-slider-control-height)
);
}
#fill,
.fill,
.track {
block-size: var(
--mod-slider-track-fill-thickness,
Expand Down Expand Up @@ -237,7 +237,7 @@ governing permissions and limitations under the License.
position: absolute;
z-index: 1;
}
#fill:before,
.fill:before,
.track:before {
block-size: 100%;
border-end-end-radius: 0;
Expand Down Expand Up @@ -303,7 +303,7 @@ governing permissions and limitations under the License.
var(--spectrum-slider-track-middle-handleoffset)
);
}
#fill {
.fill {
margin-inline-start: 0;
padding-block: 0;
padding-inline-end: 0;
Expand All @@ -315,7 +315,7 @@ governing permissions and limitations under the License.
var(--spectrum-slider-handle-gap, var(--spectrum-slider-handle-gap))
);
}
.spectrum-Slider-fill--right {
.offset {
padding-block: 0;
padding-inline-end: calc(
var(
Expand Down Expand Up @@ -737,7 +737,7 @@ governing permissions and limitations under the License.
)
);
}
#fill:before {
.fill:before {
background: var(
--highcontrast-slider-track-fill-color,
var(
Expand Down Expand Up @@ -929,7 +929,7 @@ governing permissions and limitations under the License.
)
);
}
:host([disabled]) #fill:before {
:host([disabled]) .fill:before {
background: var(
--highcontrast-slider-track-fill-color-disabled,
var(
Expand Down
63 changes: 63 additions & 0 deletions packages/slider/stories/slider.stories.ts
Expand Up @@ -132,6 +132,69 @@ export const Default = (args: StoryArgs = {}): TemplateResult => {
`;
};

export const Filled = (args: StoryArgs = {}): TemplateResult => {
return html`
<div style="width: 500px; margin-inline: 20px;">
<sp-slider
max="1"
variant="filled"
min="0"
value=".5"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, can we use a value other than the middle of the range so that this story shows the fill by default in VRT so that we can test that the output stays the same over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-01-17 at 11 34 00 AM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I put this comment on the wrong story. 😞 I meant for https://bug-slider-filled--spectrum-web-components.netlify.app/storybook/?path=/story/slider--fill-start for all the same reasons. I swear this is the last thing!

Copy link
Contributor Author

@Rajdeepc Rajdeepc Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-01-17 at 6 45 46 PM
<sp-slider
      max="1"
      fill-start
      min="0"
      value=".7"
      step="0.01"
      @input=${handleEvent(args)}
      @change=${handleEvent(args)}
      .formatOptions=${{ style: 'percent' }}
      ...=${spreadProps(args)}
>
    Slider label
</sp-slider>
            

step="0.01"
@input=${handleEvent(args)}
@change=${handleEvent(args)}
.formatOptions=${{ style: 'percent' }}
...=${spreadProps(args)}
>
Slider Label
</sp-slider>
</div>
`;
};

export const withOnlyFillStart = (args: StoryArgs = {}): TemplateResult => {
return html`
<div style="width: 500px; margin-inline: 20px;">
<sp-slider
max="1"
fill-start
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want variant="filled" here. I know this is less things, but it's also less clear. I could be convinced away, but it feels more expected for each of the "filled" sliders to have the same variant and for the fill-start attribute to change the way that the fill is delivered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we cannot use variant="fiiled" when using this property fill-start becuase of this css

:host([variant='filled']) .track:first-child:before {
    background: var(
        --highcontrast-slider-track-fill-color,
        var(
            --mod-slider-track-fill-color,
            var(--spectrum-slider-track-fill-color)
        )
    );
}

If fill-start has value then the track will be filled even if it is not starting from 0

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means we're opening a fail state in the API <sp-slider variant="filled" fill-start="15" max="20" value="5"></sp-slider>. How would you like to handle this case and support consumers to success?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this use case. This should be handled now

min="0"
value=".5"
step="0.01"
@input=${handleEvent(args)}
@change=${handleEvent(args)}
.formatOptions=${{ style: 'percent' }}
...=${spreadProps(args)}
>
Slider label
</sp-slider>
</div>
`;
};

export const withFillStartValue = (args: StoryArgs = {}): TemplateResult => {
return html`
<div style="width: 500px; margin-inline: 20px;">
<sp-slider
max="1"
min="0"
value=".7"
step="0.1"
@input=${handleEvent(args)}
@change=${handleEvent(args)}
.formatOptions=${{ style: 'percent' }}
...=${spreadProps(args)}
>
Slider label
</sp-slider>
</div>
`;
};

withFillStartValue.args = {
fillStart: 0.3,
};

export const autofocus = (args: StoryArgs = {}): TemplateResult => {
return html`
<div style="width: 500px; margin-inline: 20px;">
Expand Down