illright/attractions

[Feature Request] Slider

Closed this issue · 20 comments

Chaostheorie commented

This is a feature request for a slider component. A slider component is an input for numbers based on a slider, which consists of a line with a draggable dot on top, to select a number from a fixed range based on the relative position of the dot on the line. A good implementation of such a component is found in carbon-components-svelte and as part of the material spec.

illright commented

Hi, thanks for your request! We will take it into consideration, though I can't give you a timeframe for its implementation at the moment.

I looked at the slider at Carbon Components, would you prefer a similar API or is there something you'd wanna change about it?

Also, if you wish to contribute to the implementation of this component, you are very welcome :)

Chaostheorie commented

Thank you for the quick response. I've just recently picked up Svelte(-Kit) and have migrated from carbon-components to attractions since carbon-components was incompatible with svelte-kit.

I would love to contribute and will look into how to create the component, but I don't really have any experience, so it might take a while. Likewise, I personally would prefer a simpler API than carbon-component's slider for easier integration and to be consistent with the other UI components of attractions.

illright commented

Such as? Maybe you could list the props/slots/events you'd want to see? As for little experience, it's alright, you can fork and make a draft pull request. We'll give you feedback as you proceed

Chaostheorie commented

I'll try my best on the component. Will make an attempt tomorrow since it's already night where I'm currently.

Regarding props and events, they would, at least AFAIK, basically be the same as if someone would use <input type="number"> directly. Slider is just an abstraction layer that allows a better presentation of max and min to the user.

props:

  • max: number.
  • min?: number = 0
  • default?: number = 0
  • deactivated?: boolean = false
  • class / style

reactive

  • value (current value): number

events:

  • change
illright commented

I'd suggest the following changes to this API:
default → value (no need for an extra "default" prop)
deactivated → disabled (more conventional)

And perhaps add the step?: number prop

Chaostheorie commented

Might take me a while since other projects are taking up my time ATM. Will try to finding a first proof of concept next week

RikuVan commented

Was actually thinking to make a more complex slider that allows for more varied usage and I checked in here thinking I might suggest it for Attractions. Anyways, I would be willing to help out as needed as, I have done a few of these before.

Chaostheorie commented
RikuVan commented

I have started working on a versatile slider bit by bit, which I might try to contribute but it will be some weeks.

RikuVan commented

Ok, spent some free time in the last days whip out a raw first version: https://github.com/RikuVan/svelte-sliders. I thought I would do a POC outside of the attractions repo first to see if really is the kind of thing that would fit in attractions.

RikuVan commented

Never used JSDoc for type checking before. Maybe I am doing it wrong but it didn't seem to help me much--was missing typescript. Also didn't really put much effort into the styles. I made the TimeSlider in addition to the basic one because I have a need for this component.

RikuVan commented

Could @illright @aabounegm comment, could something along these longs work as a PR: https://github.com/RikuVan/svelte-sliders

aabounegm commented

@RikuVan I love it! You clearly put a ton of effort into it! Your JSDoc is on point, and I applaud your care about accessibility. The demo is amazing, and so are the styles (but @illright is a better judge about whether they fit Attractions or need slight modifications). This should definitely be turned into a PR, and I just have some comments to be addressed:

  • I don't like the fact that there are many extremely small components I am not sure why they are separate. For example, the Rail consists of a single <div> with just a slot that is not even used. I believe SCSS can be organized and properly nested in a way that makes it no reason to split the component.
  • The usage of context store to pass down data is a consequence of the previous point that adds more complexity to the code, and even that usage can still be accomplished using normal props (since each component only uses a small subset of the Slider's props).
  • Instead of the map function used for the ticks, it might be better to use scoped slots for the ticks and the handle tooltip. We can even go as far as allowing different slots for each, with the tooltip one defaulting to the tick's (since one named slot can be used multiple times).
  • The TimeSlider is just a normal Slider with customized ticks labels. I do not think it calls for a separate component altogether.
  • I believe that the dependency on date-fns can be easily replaced with native APIs (and I see you only use it for the TimeSlider anyway).
  • To maintain consistency with other components in the library, it will be better if you replace the type unions (such as type Mode = 'none' | 'steps' | 'values';) with an actual enum, as with PopoverPositions.
  • I'd replace the orientation prop with a boolean vertical prop defaulting to false. It just feels like more natural usage to me.
  • I don't get the purpose of the dragOffset prop.
  • When getting rid of the context store, we also wouldn't need the createId function, and the component's id attribute can be just part of $$restProps (which I don't see being used)
  • Can the combination of mouse/touch events with sliderActive be replaced with drag events instead?
  • Any SCSS variables?

I am also thinking it should be separated into Slider (1 handle) and RangeSlider (2 handles) as they have a slightly different APIs. What do you think?

I hope you are not overwhelmed by these comments (feel free to just create the PR and I can jump right in with the modifications), and we really appreciate the effort you put into implementing this. You are amazing 🌟

RikuVan commented

Thanks for your comments and encouragement. I can't answer all these now, although some are more easily answered than others. Here are some out-of-order thoughts.

  • regarding the TimeSlider, I didn't really intend this one for attractions--you can ignore that and its dependencies.

  • yes, I think a lot of scss variables still need to be extracted and the styles need work.

  • have to try out if slots are a reasonable solution for ticks, etc. These all need to be precisely positioned to it may be tricky for a consumer to get a slot correct, unless it is only the content and not the container.

  • some of the architectural differences are debatable but I don't mind giving into the libraries preferences for these. I generally prefer components to be broken into their constituent elements because it makes then exportable and composable into new versions by consumers. I think it also makes it easier to reason about. So maybe, for instance, I want to use the Slider container but I want to use my own ticks or handles implementations entirely. This can also make testing easier as well but right now there don't seem to be unit tests in attractions 😓 .This "breaking into composable parts" is quite common in the React world where I have been working for 6-7 years. But I can do it as one large singe-file component if needed. This then makes the question of passing props versus using a store (which I think has advantages) beside the point, since there is nothing to pass. One thing I would less enthusiastic about is moving all the helper functions into the component - I just think it because a mess in the script tags--but I could be talked into this I suppose.

  • I am not sure what you mean about using an actual enum instead of a union since this was not in typescript (would be happy if it was :))?

  • touch events are better on mobile than drag events. generally you will find a combination of mouse and touch events, or then only touch events, on most interactive components I think. Drag might be useful feature that might be worth adding where you want to drag the connected section or range between two handles as a whole section.

I will try to work this into an initial PR when I find another window of time. Any more suggestions are welcome.

RikuVan commented

btw i can separate into <Slider /> and <Range /> components, but then it will make sense to have the common parts as separate components. I also thought maybe to add an alternative handle design to the simple round one, that could be set via a prop - this is now seen in the first example in the demo. I added sub-ticks this morning which are now in the demo.

illright commented

I'm also very impressed with this demo.

Regarding the styles: I do like the Slider with the round handles, especially when it doesn't have the subticks – so minimal, so cool. I would give a lighter gray to the slider bar, but all of these things could be easily customizable with Sass variables.

Regarding the composability: is that BEM methodology that you're using? I must say I believe that BEM fits very well with React, but not so much with Svelte, where you don't get much control over the children, so your composability options are limited. That's partially the reason why the components of Attractions aren't broken up so finely as they are in your demo. However, I may be biased and/or mistaken. If you could demonstrate how we could achieve composability with this setup, I'd gladly listen.

RikuVan commented

I agree, BEM is not needed. Just kind of put something in a bit of a hurry.

RikuVan commented

Have started putting this component in attractions. Have run into some annoying problems with sveld, regarding slots. If fails to compile if a forwarded slot has the same name as the slot in the child. Well this is not such a big deal. But another problem is the forwarded props. I get (plugin plugin-sveld) TypeError: Cannot read property '0' of undefined when compiling types, which looks a lot like an old error in Svelte so maybe this is to do with the version of Svelte sveld is using. dunno. Not sure how to get around this.

RikuVan commented

Ok will solve the above by lifting up the slots to the parent so there is no forwarding

RikuVan commented

moved to PR #290