Add divisions property to ZenitSlider #21
Conversation
| this.sliderTheme, | ||
| }) : assert( | ||
| value >= 0.0 && value <= 1.0 && divisions == null || | ||
| divisions != null && divisions > 0 && divisions <= 100, |
There was a problem hiding this comment.
why is there an upper limit for divisions?
There was a problem hiding this comment.
I didn't think it makes sense to have divisions for less than a percent
| }, | ||
| child: GestureDetector( | ||
| onTapDown: (details) { | ||
| newValue = details.localPosition.dx / (constraints.maxWidth); | ||
| widget.onChanged(newValue); | ||
| if (widget.divisions != null) newValue = (newValue / divident).round() * divident; | ||
| if (newValue >= 0.0 && newValue <= 1.0) widget.onChanged(newValue); |
There was a problem hiding this comment.
does this check make sense to have? maybe it'd be better to clamp the newValue passed to onChanged instead. also this is repeated logic, maybe it can be moved in a separate method
There was a problem hiding this comment.
You mean line 52, correct?
| double activeWidth() { | ||
| if (size.width * value >= size.width) { | ||
| return size.width; | ||
| } else if (size.width * value < 24) { | ||
| return 24; | ||
| } else { | ||
| return size.width * value + 12; | ||
| } | ||
| } | ||
|
|
||
| double thumbPositionX() { | ||
| if (size.width * value >= size.width) { | ||
| return size.width - 12; | ||
| } else if (size.width * value < 24) { | ||
| return 12; | ||
| } else { | ||
| return size.width * value; | ||
| } | ||
| } |
There was a problem hiding this comment.
instead of having those cumbersome if/else statements i'd either switch to a switch/case expression or just clamp value so we know it won't overshoot or undershoot the position. also prefer having external methods where you pass in size instead of these methods inside methods
There was a problem hiding this comment.
I believe a switch case would not work there as size is not constant
There was a problem hiding this comment.
using gates this can work easily, tho to simplify just make return ((size.width - 24.0) * value.clamp(0.0, 1.0)) + 12.0;, same thing for the other one
| } | ||
| } | ||
|
|
||
| const kTrackBorderRadius = Radius.circular(12.0); |
There was a problem hiding this comment.
a lot of "magic" values, 12 and 24. wouldn't it make sense to have a top level private constant with these values? you could just have something like "_kSliderHeight = 24.0" and another constant which would be like "_kSliderRadius = _kSliderHeight / 2", this would also make it easier to make customizable eventually
|
|
||
| const kTrackBorderRadius = Radius.circular(12.0); | ||
|
|
||
| final RRect track = RRect.fromLTRBR(0.0, 12.0, size.width, size.height - 12, kTrackBorderRadius); |
There was a problem hiding this comment.
floating point formatting consistency, you use .0 everywhere but here
| for (int i = 0; i < divisions - 1; i++) { | ||
| final double x = (size.width / divisions) * i + (size.width / divisions); | ||
| final Offset position = Offset(x, size.height / 2); | ||
| canvas.drawCircle(position, 2, dividerPaint); |
There was a problem hiding this comment.
is the divider row always painted in front of the track? if yes, is it always visible? could make sense to have some blend modes to aid visibility
| const Radius.circular(12.0), | ||
| ); | ||
| final Paint dividerPaint = Paint() | ||
| ..color = outlineColor |
There was a problem hiding this comment.
outlineColor for the dividers doesn't make a lot of sense, could you rename this to something better? and if it's used by something else, maybe look into decoupling to allow customizability?
There was a problem hiding this comment.
Oh yeah, seems like I forgot to change that
No description provided.