fix(slider): added filled offset variant #3876
Conversation
Rajdeepc
changed the title
Tachometer resultsChromeslider permalink
Firefoxslider permalink
|
Rajdeepc
force-pushed
the
bug/slider-filled
branch
2 times, most recently
from
3b9073b
to
c067b70
Compare
packages/slider/src/Slider.ts
Outdated
| @@ -160,6 +164,9 @@ 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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
should min the start value or the initial value represented as 'value' ?
fillStart boolean creates the filled highlight on the track.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Sure I was wondering if fill-start="5" and value="6" then how will the slider behave? which takes precendence?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
-
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? -
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 fromvalue="15"itself? And also do we show fill from 5-15 only or from 5-0 also?
There was a problem hiding this comment.
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=====---------------|
There was a problem hiding this comment.
Attached video recording for preview and review. Let me know how this solution looks now.
Westbrook
changed the title
packages/slider/README.md
Outdated
| <script type="module"> | ||
| const initSlider = async () => { | ||
| const slider = document.querySelector('#fill-start-slider'); | ||
| slider.fillStart = 0.3 |
There was a problem hiding this comment.
Can we not apply this with the fill-start attribute?
There was a problem hiding this comment.
Adjusted the logic to add fill-start as an empty attribute as well as with a value
packages/slider/src/Slider.ts
Outdated
| if (typeof this.fillStart === 'number') { | ||
| this.fillStartPoint = this.fillStart; | ||
| } else { | ||
| this.fillStartPoint = | ||
| (Number(this.max) - Number(this.min)) / 2 + | ||
| Number(this.min); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this just be a getter instead? That would allow us to not place fillStartPoint as a reactive property, as well.
There was a problem hiding this comment.
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);
}
}
packages/slider/src/Slider.ts
Outdated
| class=${`fill ${ | ||
| !!(this.value > this.fillStartPoint) ? 'offset' : '' | ||
| }`} |
There was a problem hiding this comment.
using classMap({fill: true, offset: this.value > this.fillStartPoint}) might make this easier to read.
packages/slider/src/Slider.ts
Outdated
| 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 | ||
| )}%`, | ||
| })} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This has been updated to make it more modular
| <div style="width: 500px; margin-inline: 20px;"> | ||
| <sp-slider | ||
| max="1" | ||
| fill-start |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks for pointing this use case. This should be handled now
packages/slider/test/slider.test.ts
Outdated
| await nextFrame(); | ||
|
|
||
| expect(el.value).to.equal(24); | ||
| expect(el.shadowRoot.querySelector('.fill') as HTMLDivElement).to.exist; |
There was a problem hiding this comment.
Probably a good idea to assert that .fill doesn't exist to get started with?
There was a problem hiding this comment.
class fill will be there when fill-start or fill-start with value is there with width as 0% when only fill-start
There was a problem hiding this comment.
Then a presence check is likely not enough to actually guarantee that the role that element is playing is complete to the situation in question.
Rajdeepc
force-pushed
the
bug/slider-filled
branch
from
3bb43b2
to
524493d
Compare
… into bug/slider-filled
packages/slider/src/Slider.ts
Outdated
| if (fillStartValue === cachedValue) { | ||
| distance = Math.abs(currentValue - fillStartValue); | ||
| } else { | ||
| distance = Math.abs(currentValue - fillStartValue); | ||
| } |
There was a problem hiding this comment.
This seems like the same on both sides of the logic.
| max="1" | ||
| variant="filled" | ||
| min="0" | ||
| value=".5" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
<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>
|
Thanks for checking @Westbrook. My use case only requires the vanilla |
🥳
Description
Add fill-start/fill-offset functionality to Slider
Related issue(s)
Motivation and context
How has this been tested?
Screenshots (if appropriate)
fillslider.mov
With only fill-start, fill starting from center
With a fill-start value, fill starting from fill-start value
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.