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

sl-multi-range component #1912

Draft
wants to merge 19 commits into
base: next
Choose a base branch
from

Conversation

mpharoah
Copy link
Contributor

@mpharoah mpharoah commented Mar 7, 2024

Implements #616
Currently, Shoelace implements an sl-range component for a simple slider with one value.

This pull request is a prototype for a new similar component that differs in that it supports more than one value/handle. The most common use case is using two values to select a range, but the component supports an arbitrary number of handles.

There aren't any unit tests, but everything else is now implemented

image

Copy link

vercel bot commented Mar 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview Mar 25, 2024 10:24pm

@claviska
Copy link
Member

claviska commented Mar 8, 2024

Thanks for the suggestion! To save yourself effort in the future, it's almost always best to open a discussion to talk about potential new features before doing the work :)

That said, I don't think a slider with multiple thumbs is out of the question, but we'll probably want to incorporate it into a single component. The biggest question about supporting this was how do we handle form submissions? The control will have one name, so the value will probably have to be delimited, e.g. 12,74 or 12,74,98 for multiple thumbs.

Pinging @KonnorRogers since he's done some work with vertical sliders before, which is another feature we'd like to add. Of course, it will require a rewrite of the existing component. Perhaps we can use @mpharoah's work as a baseline or at least for inspiration when that time comes.

…sed to move them, even if they were originally focused via the mouse cursor
@mpharoah
Copy link
Contributor Author

Thanks for the suggestion! To save yourself effort in the future, it's almost always best to open a discussion to talk about potential new features before doing the work :)

No problem. I needed this component for my site anyway, so I had to do the work either way. Just figured I might as well contribute what I had done back to Shoelace since porting from my component to Shoelace was pretty trivial.

I'll try to get tooltips added since that would be a nice-to-have for me, then I'll leave this here for you to do whatever you want with it, if anything

@mpharoah
Copy link
Contributor Author

Okay, tooltips work now. Aside from missing HTML Form integration and css parts, it should now have everything it needs. There's definitely some code style differences between this PR and other Shoelace components (ie. using actual private members, css names, and using willUpdate instead of @watch), but the actual functionality shouldn't need to change much if at all if you want to officially bring this into Shoelace at a later point.

@mpharoah mpharoah changed the title sl-multi-range prototype sl-multi-range component Mar 11, 2024
aria-disabled=${ifDefined(this.disabled ? 'true' : undefined)}
aria-valuenow="${value}"
data-slider-id="${sliderId}"
@pointerdown=${this.#onClickHandle}
Copy link
Contributor Author

@mpharoah mpharoah Mar 11, 2024

Choose a reason for hiding this comment

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

I use the pointer* events instead of mouse events here because it gracefully and automatically handles edge cases like releasing the mouse button when the cursor is no longer in the browser window. It also means that you should be able to move multiple sliders at once on multi-tap devices (Though the tooltip will only show over one of them).

@schilchSICKAG
Copy link
Contributor

We currently also have the requirement of adding a slider with multiple knobs. I think this version looks quite promising. However, we would also like to have various other options like:

  • Knobs should be their own component (e.g. sl-multi-range-knob) that can be slotted into the sl-multi-range. This would make usage more declarative and you would be free to add as many knobs as you want. See Spectrum Web Components for an example of this where the knobs even handle collision detection and boundaries, making the component sl-multi-range-knob more lightweight.
  • It would also be nice to have the possibility to set an unlimited number of tick markers. This would allow the user to either go with another scale as a default linear one.

Is there any way we could support from our side to make this component land in shoelace?

@claviska @mpharoah

@claviska
Copy link
Member

Knobs should be their own component (e.g. sl-multi-range-knob) that can be slotted into the sl-multi-range

This feels like a premature abstraction. The knob will be a single element with a handful of styles. Making it a full blown component would be overkill and result in more bytes for little perceived benefit. What's your use case for it?

It would also be nice to have the possibility to set an unlimited number of tick markers.

Noted, thanks. We haven't begun to properly scope this feature out, but this will be helpful during that phase.

Is there any way we could support from our side to make this component land in shoelace?

We're currently focusing on the Web Awesome alpha early release for Kickstarter backers, so this is on pause for the moment. @KonnorRogers has already done the work internally to move Shoelace form controls to form-associated elements in Web Awesome, so this PR is behind because it lacks our new base class and utilities to make it a first-class form control.

We'll get there. Lots going on right now. Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants