Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add divisions property to ZenitSlider #21

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add divisions property to ZenitSlider #21

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fab511
Add divisions property to ZenitSlider
larsb24 Jan 30, 2024
c15ba3f
Refactor slider component and add second example
larsb24 Jan 31, 2024
1de4f45
Change value to divisionsWidthRatio an add dividerColor property to p…
larsb24 Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions example/lib/pages/slider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,34 @@ class SliderExample extends StatefulWidget {

class _SliderExampleState extends State<SliderExample> {
double value = 0.5;
double value2 = 0.5;

@override
Widget build(BuildContext context) {
return Center(
child: Padding(
padding: const EdgeInsets.all(32.0),
child: ZenitSlider(
onChanged: (val) {
setState(() {
value = val;
});
},
value: value,
),
return Padding(
padding: const EdgeInsets.all(32.0),
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
children: [
ZenitSlider(
divisions: 10,
onChanged: (val) {
setState(() {
value = val;
});
},
value: value,
),
const SizedBox(height: 32),
ZenitSlider(
onChanged: (val) {
setState(() {
value2 = val;
});
},
value: value2,
),
],
),
);
}
Expand Down
95 changes: 62 additions & 33 deletions lib/src/components/slider/slider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,38 @@
import 'package:flutter/material.dart';

Check warning on line 1 in lib/src/components/slider/slider.dart

View workflow job for this annotation

GitHub Actions / ZenitUI Code Analysis 

Invalid format. For more details, see https://dart.dev/guides/language/effective-dart/style#formatting
import 'package:zenit_ui/src/theme/theme.dart';

class ZenitSlider extends StatefulWidget {
final double value;
final ValueChanged<double> onChanged;

final Color? activeColor;
// TODO: implement divisions
//final int? divisions;
final Color? trackColor;
final int? divisions;
final MouseCursor? mouseCursor;
final Color? thumbColor;
final ZenitSliderTheme? sliderTheme;

const ZenitSlider({
super.key,
required this.value,
required this.onChanged,
this.activeColor,
//this.divisions,
this.trackColor,
this.divisions,
this.mouseCursor,
this.thumbColor,
}) : assert(value >= 0.0 && value <= 1.0);
this.sliderTheme,
}) : assert(
value >= 0.0 && value <= 1.0 && divisions == null ||
divisions != null && divisions > 0 && divisions <= 100,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there an upper limit for divisions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think it makes sense to have divisions for less than a percent

);

@override
State<ZenitSlider> createState() => _ZenitSliderState();
}

class _ZenitSliderState extends State<ZenitSlider> {
bool hover = false;

@override
Widget build(BuildContext context) {
final sliderTheme = ZenitTheme.sliderTheme(context);
final sliderTheme = widget.sliderTheme ?? ZenitTheme.sliderTheme(context);
double newValue = widget.value;
final double divident = 1 / (widget.divisions ?? 1);
larsb24 marked this conversation as resolved.
Show resolved Hide resolved
return LayoutBuilder(
builder: (context, constraints) {
return MouseRegion(
Expand All @@ -42,28 +42,30 @@
child: Listener(
onPointerPanZoomStart: (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);
},
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean line 52, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea

},
onHorizontalDragUpdate: (details) {
newValue += details.delta.dx / constraints.maxWidth;
if (newValue >= 0.0 && newValue <= 1.0) {
widget.onChanged(newValue);
}
newValue = details.localPosition.dx / (constraints.maxWidth);
if (widget.divisions != null) newValue = (newValue / divident).round() * divident;
if (newValue >= 0.0 && newValue <= 1.0) widget.onChanged(newValue);
},
child: CustomPaint(
painter: _SliderPainter(
trackColor: widget.trackColor ?? sliderTheme.trackColor,
activeColor: widget.activeColor ?? sliderTheme.activeTrackColor,
thumbColor: widget.thumbColor ?? sliderTheme.thumbColor,
trackColor: sliderTheme.trackColor,
activeColor: sliderTheme.activeTrackColor,
thumbColor: sliderTheme.thumbColor,
hover: hover,
hoverColor: Theme.of(context).colorScheme.onSurface.withOpacity(0.05),
outlineColor: sliderTheme.outlineColor,
value: widget.value,
divisions: widget.divisions,
),
size: Size(constraints.maxWidth, 48),
),
Expand All @@ -83,6 +85,7 @@
final Color hoverColor;
final Color thumbColor;
final Color outlineColor;
final int? divisions;

_SliderPainter({
required this.trackColor,
Expand All @@ -92,6 +95,7 @@
required this.hoverColor,
required this.thumbColor,
required this.outlineColor,
this.divisions,
});

@override
Expand All @@ -113,25 +117,49 @@
..style = PaintingStyle.stroke
..strokeWidth = 1;

final RRect track = RRect.fromLTRBR(0.0, 12.0, size.width, size.height - 12, const Radius.circular(12.0));
final RRect active = RRect.fromLTRBR(
0.0,
12.0,
(size.width * value) > 24 ? (size.width * value) : 24.0,
size.height - 12.0,
const Radius.circular(12.0),
);
final Paint dividerPaint = Paint()
..color = outlineColor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, seems like I forgot to change that

..style = PaintingStyle.fill;

final Offset thumbPosition = Offset(
(size.width * value) > 24 ? (size.width * value) - ((size.height - 26) / 3) - 6 : 24 - 12,
size.height / 2,
);
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;
}
}
Comment on lines +133 to +151
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe a switch case would not work there as size is not constant

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


final RRect track = RRect.fromLTRBR(0.0, 12.0, size.width, size.height - 12, kTrackBorderRadius);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

floating point formatting consistency, you use .0 everywhere but here

final RRect active = RRect.fromLTRBR(0.0, 12.0, activeWidth(), size.height - 12.0, kTrackBorderRadius);

final Offset thumbPosition = Offset(thumbPositionX(), size.height / 2);

canvas.drawRRect(track, trackPaint);
canvas.drawRRect(track, outlinePaint);

canvas.drawRRect(active, activePaint);

final int divisions = this.divisions ?? 1;
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if (hover) {
final Paint hoverPaint = Paint()
..color = hoverColor
Expand All @@ -151,6 +179,7 @@
old.hover != hover ||
old.hoverColor != hoverColor ||
old.thumbColor != thumbColor ||
old.outlineColor != outlineColor;
old.outlineColor != outlineColor ||
old.divisions != divisions;
}
}
5 changes: 5 additions & 0 deletions lib/src/theme/theme.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ mixin ZenitTheme {
trackColor: theme.colorScheme.surface,
thumbColor: theme.colorScheme.onPrimary,
outlineColor: theme.colorScheme.outline,
dividerColor: theme.colorScheme.outline,
);
}

Expand Down Expand Up @@ -262,25 +263,29 @@ class ZenitSliderTheme {
final Color trackColor;
final Color outlineColor;
final Color thumbColor;
final Color dividerColor;

const ZenitSliderTheme({
required this.activeTrackColor,
required this.trackColor,
required this.outlineColor,
required this.thumbColor,
required this.dividerColor,
});

ZenitSliderTheme copyWith({
Color? activeTrackColor,
Color? trackColor,
Color? outlineColor,
Color? thumbColor,
Color? dividerColor,
}) {
return ZenitSliderTheme(
activeTrackColor: activeTrackColor ?? this.activeTrackColor,
trackColor: trackColor ?? this.trackColor,
outlineColor: outlineColor ?? this.outlineColor,
thumbColor: thumbColor ?? this.thumbColor,
dividerColor: dividerColor ?? this.dividerColor,
);
}
}
Expand Down
Loading