Slider improvements

[jjenzz]

It's probably easier to just review these files from scratch. Don't worry about reviewing stuff I haven't changed, I'm happy to rework it too if we think that's needed.

• Fixes bugs Slider: clicking on track doesn't move thumb #126 and Slider: keyboard only works after having clicked the Thumb #127
• Adds horizontal/vertical orientation
• Removes redundant size listeners that were a hangover from editor days
• Tidies up the code to make things a bit easier to follow

---

Note: There is a TS error to do with our forwardRef typing. I'm trying to remove defaultValue from the React.componentPropsWithRef<'span'> type, but our forwardRef stuff adds it back in. This results in a never type for defaultValue when using the array variation

I tried to fix it before PRing but it opened a can of worms. I'm not sure if it's worth investing too much time here given the polymorphic work I've been doing separately but we do need to fix that before releasing it.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/modulz/interop-ui/oh1rix4uy/modulz-deploys.com
✅ Preview: https://interop-ui-git-slider-fixes.modulz-deploys.com

[Deployment for 1b04b84 failed]

[benoitgrelard]

This is much simpler to review now so, nice job! 👏

I have a few general comments below (as well as a handful smaller inline ones).

---

Where's the TS error? I don't seem to see it.

---

There's something that doesn't feel quite right for me with regards to arrow keys.
I'm not sure I'm right about it but it feels strange.

• horizontal orientation, left-right is good, but up-down seem inverted to me
• vertical orientation, up-down is good, left-right seems good but only because the range starts from the top, what if we want the range to start from the bottom? I feel like it should be possible, like an equalizer-type UI.

Note: I saw that you have a startEdge/endEdge in context, I think it's mainly to execute horizontal vs. vertical, but I feel like a start edge could be useful in achieving what I've described above as well as dealing with arrows accordingly.

---

Generally, there are quite a few functions that take a few args, often most of them numbers, which I feel make them harder to track and prone to mistakes. I generally switch to an object of named params for these. Not sure what you think?

---

I completely get what you've tried to do with SliderHorizontal and SliderVertical to avoid the orientation ? … : … in a bunch of places. I don't know if I prefer it though to be honest. This way gives me a big indirection from the top level. I feel like I'd rather have one component where I see everytime we switch from width to height, top to left, clientX to clientY, etc. Doesn't seem to be many of them. I usually prefer inlining like this. I say this knowing fully that you've probably considered it and tried it, but that's just my gut feeling when I see that it's mostly duplicated code.

Note: I think that perhaps this came from being too much when everything was in one place (including the abstractions now built into SliderPart). I suspect a good middle ground now would be to retain the SliderPart abstraction, but close down SliderVertical and SliderHorizontal into Slider.

[jjenzz]

@benoitgrelard

There's something that doesn't feel quite right for me with regards to arrow keys.

Ah yeah, I see that. Admittedly, I didn't have the up/down keys working in horizontal and vice versa initially, but I thought it would simplify things to just allow them. Clearly not. I'll just prevent them again in the orientations they're not required.

vertical orientation, [...] what if we want the range to start from the bottom? I feel like it should be possible, like an equalizer-type UI.

Hmmm, this is an interesting one. That implies we should be able to start from the right too in horizontal mode. What are your thoughts on this one @colmtuite @chaance ? It seems fair to me, but just checking before I spend a bunch of time on it.

Generally, there are quite a few functions that take a few args, often most of them numbers

I agree, the fact that a lot of this stuff is even abstracted hurts my head. There was feedback to abstract these but I think we've gone a bit tooooo far. I'll move the ones that aren't being re-used back inline (where is makes sense) and try clean up some of the args for the others.

I completely get what you've tried to do with SliderHorizontal and SliderVertical to avoid the orientation ? … : … in a bunch of places. I don't know if I prefer it though to be honest. This way gives me a big indirection from the top level. I feel like I'd rather have one component where I see everytime we switch from width to height, top to left, clientX to clientY, etc. Doesn't seem to be many of them.

Yeah, I had a load of branching before and honestly, I wasn't keen at all. It wasn't easy to visualise exactly how things were going to be in a particular orientation because the orientation concerns were spread all over. With this, I can see exactly what each orientation is responsible for. However, I have changed things quite a bit since my initial branching madness SO... I'll create a separate temp branch where we can compare the two and see which we prefer.

[chaance]

Ah yeah, I see that. Admittedly, I didn't have the up/down keys working in horizontal and vice versa initially, but I thought it would simplify things to just allow them. Clearly not. I'll just prevent them again in the orientations they're not required.

I actually think we want all directional keys working in either orientation. That's now native range inputs work (only horizontal mode for native, obviously).

• Horizontal: ArrowRight and ArrowLeft move along with visual expectation. ArrowDown aways decrements in LTR reading/writing mode and always increments in RTL, vice versa for ArrowRight
• Vertical: ArrowUp and ArrowDown move along with visual expectation. ArrowLeft decrements and ArrowRight increments. Not sure how reading/writing mode plays with vertical sliders, though I suspect it may follow the logic of horizontal sliders.

[chaance]

Hmmm, this is an interesting one. That implies we should be able to start from the right too in horizontal mode. What are your thoughts on this one @colmtuite @chaance ? It seems fair to me, but just checking before I spend a bunch of time on it.

RTL is important for internationalization, so I definitely think we should support it. I generally expect vertical sliders to start from the bottom, but that's a little less clear since we operate in a horizontal reading mode in english. I do think we need to support all cases since both vertical and RTL languages will need one or the other depending on its context.

[benoitgrelard]

Ah yeah, I see that. Admittedly, I didn't have the up/down keys working in horizontal and vice versa initially, but I thought it would simplify things to just allow them. Clearly not. I'll just prevent them again in the orientations they're not required.

I actually think we want all directional keys working in either orientation. That's now native range inputs work (only horizontal mode for native, obviously).

• Horizontal: ArrowRight and ArrowLeft move along with visual expectation. ArrowDown aways decrements in LTR reading/writing mode and always increments in RTL, vice versa for ArrowRight
• Vertical: ArrowUp and ArrowDown move along with visual expectation. ArrowLeft decrements and ArrowRight increments. Not sure how reading/writing mode plays with vertical sliders, though I suspect it may follow the logic of horizontal sliders.

Yeah I think we need both, but I also think there's somehow a different expectation based on which side the range starts.
Also, horizontally RTL might actually be the same thing as I'm mentioning although I was more mentioning it for vertical (like a fader in an audio app).

[chaance]

Some questions within. Still need to give it a test run locally but I'll submit another review shortly!

[chaance]

One very minor thing but GTG!

[jjenzz]

Thanks @chaance... I'll wait for @benoitgrelard to double check he's happy with the final result too and then merge next week 👍

[benoitgrelard]

Changes are looking good!
A few things noted inline, almost there!

[benoitgrelard]

👍 🎉 🚀

[chaance]

🚢🚢🚢