Conversation
nvlaarhoven
force-pushed
the
slider#2
branch
2 times, most recently
from
008d77f to
ab7f3d4
Compare
| step: number | ||
| ) => { | ||
| const totalRange = max - min; | ||
| return (step - min) / totalRange; |
There was a problem hiding this comment.
You should guard this in case the value is zero.
| ); | ||
|
|
||
| const initialValue = { | ||
| from: props.initialValue ? props.initialValue.from : min, |
There was a problem hiding this comment.
This initial value stuff should probably do some smart checking if the min is larger than the max stuff. Same for the min and max values themselves. Otherwise we may end up with a weird inverted slider.
Thinking about it... we may also have issues with negative numbers. What happens if the min is -100 and the max is 0?
There was a problem hiding this comment.
Agree we should check if from < to and probably also if initial values are within range 👍
I checked and slider with negative values works absolutely fine.
There was a problem hiding this comment.
Don't think we can check if min is smaller than max using TS so have just made sure the initialValues are correct
Co-Authored-By: Jake 'Sid' Smith <jake@dabapps.com>
Co-Authored-By: Jake 'Sid' Smith <jake@dabapps.com>
Math.ceil won't work when the value is an integer
Which can be used in onHandleFrom and onHandleTo
Instead of onChange at end of slider movement
JakeSidSmith
left a comment
JakeSidSmith
left a comment
There was a problem hiding this comment.
We should be able to check and handle from/to and min/max being smaller than one another, or dividing by zero.
Also, what I meant when we discussed this in Slack was that the onChange should be what is now called onSlide, as this is standard for inputs. Just that it shouldn't only be called when released, but called every time the value changes.
Finally: I didn't initially check the markup of the component, but I now realize I don't know why there is a background element, let alone 2. The background color should just be applied to the wrapper? And we could remove the getControlStyle logic completely if the controls are just rendered inside the range (position absoluted to either side).
| transform: translate(-30%, -50%); | ||
| text-align: center; | ||
| background-color: @slider-control-popover-background; | ||
| font-size: 11px; |
There was a problem hiding this comment.
| font-size: 11px; | |
| font-size: @font-size-small; |
| top: -1.4em; | ||
| width: 2.5em; | ||
| height: 2em; | ||
| transform: translate(-30%, -50%); |
There was a problem hiding this comment.
| top: -1.4em; | |
| width: 2.5em; | |
| height: 2em; | |
| transform: translate(-30%, -50%); | |
| top: 0; | |
| left: 50%; | |
| transform: translate(-50%, -100%); |
| top: 50%; | ||
| left: 0; | ||
| float: left; | ||
| transform: translate(-100%, -50%); |
There was a problem hiding this comment.
| transform: translate(-100%, -50%); | |
| transform: translate(-50%, -50%); |
| height: 100%; | ||
| background-color: @slider-bar-background; | ||
|
|
||
| &:last-of-type { |
There was a problem hiding this comment.
Is there more than one background?
2 participants
Trello: https://trello.com/c/MVf2vg4g/74-back-port-slider-functionality-from-fare-comparer-into-roe-sliders