search

daisyui / react-daisyui

daisyUI components built with React 🌼
http://react.daisyui.com/
MIT License
886 stars 98 forks source link

Range component UI issues #437

Closed itaiperi closed 6 months ago

itaiperi commented 6 months ago

Hi, I'm using the Range component, and ran into two issues:

  1. I want to control the step size of the Range component, but without having the ticks - this is not possible at this point. There should be an optional parameter displayTicks or something similar for this, in my opinion. We can default it to true to keep backwards compatibility if required.
  2. The ticks don't take into account the min and max values of the Range component, which causes issues. For example, let's say I have min={10} max={20} step={1}, there should technically be 11 ticks, but the current code produces 101 ticks. It seems from the code that there was intention to use the max value, due to the useMemo dependency on it (here).

I'd be happy to create a PR with the fixes, just need to decide on a course of action here :)

itaiperi commented 6 months ago

On second thought, maybe it's even better to disconnect the step size from the ticks entirely. Have a step parameter, and a tickStep (or similar) parameter (which can default to step if needed). This provides more control. For example, as the code is right now, I can choose step size to be 0.2 if I need that in my range, but the ticks would be of size 1, because of the Math.max function.

benjitrosch commented 6 months ago

@itaiperi sorry for the late reply, you caught be right as I went on vacation!

You're totally right, the maths in the Range component are totally messed up for ticks that aren't within the [1...100] range with integer increments. And furthermore the fact we can't change the step size without displaying ticks seems like a major annoyance.

I like your idea of both adding a prop for displayTicks as well as decoupling the logic of steps from the displayed ticks. However, I would default it to having the steps and displayed ticks match unless a separate ticksStep prop is passed in, otherwise it should follow the step prop. But feel free to submit a PR with whatever you think is the cleanest/most flexible API choice and I'm happy to take a look and get this fix merged in ASAP!

itaiperi commented 6 months ago

@benjitrosch yeah, that was my original meaning, should have been clearer. I'll submit a PR and we'll take it from there