[rfw] Material slider widget #6610
Conversation
- Added tests
| onChanged: source.handler(['onChanged'], | ||
| (HandlerTrigger trigger) => (double value) { | ||
| trigger({'value': value}); | ||
| }), |
There was a problem hiding this comment.
Per the style guide, indentation should never have a line that is less indented than a line that is of higher lexical scope (well the style guide doesn't say it quite that way but that's the intent).
| onChanged: source.handler(['onChanged'], | |
| (HandlerTrigger trigger) => (double value) { | |
| trigger({'value': value}); | |
| }), | |
| onChanged: source.handler(['onChanged'], | |
| (HandlerTrigger trigger) => (double value) { | |
| trigger({'value': value}); | |
| }, | |
| ), |
| final min = source.v<double>(['min']) ?? 0.0; | ||
| final value = source.v<double>(['value']) ?? min; | ||
| final labelText = source.v<String>(['label']); | ||
| final label = labelText != null ? '$labelText:${value.toStringAsFixed(2)}' : value.toStringAsFixed(2); |
There was a problem hiding this comment.
should there be a space after the :?
packages/rfw/test/utils.dart
Outdated
| @@ -21,3 +22,15 @@ bool get isMainChannel { | |||
|
|
|||
| // See Contributing section of README.md file. | |||
| final bool runGoldens = !kIsWeb && Platform.isLinux && isMainChannel; | |||
|
|
|||
| // slide to value for material slider in tests | |||
| extension SlideTo on WidgetTester { | |||
There was a problem hiding this comment.
can you make this a function instead of an extension? that would make it easier to find when reading the code (otherwise it looks like you have to look on WidgetTester).
See also https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-using-extension
|
This looks great! Sorry it took me so long to review! Just some minor nits. |
- Changed slideToValue to be function instead of extension - Added space after : in material slider label default text
|
Please let me know if there is something else I can fix after latest changes |
|
auto label is removed for flutter/packages/6610, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.
|
auto-submit
bot
removed
the
autosubmit
|
Thank you for the approval. Do we need to wait for another approval before this ends up in version 1.0.27 or how does it work from here? |
|
Oh I forgot it needed a second review. Let me post about it in the Discord. Sorry about that. Yay for the bots! |
| final min = source.v<double>(['min']) ?? 0.0; | ||
| final value = source.v<double>(['value']) ?? min; | ||
| final labelText = source.v<String>(['label']); | ||
| final label = labelText != null ? '$labelText: ${value.toStringAsFixed(2)}' : value.toStringAsFixed(2); |
There was a problem hiding this comment.
Should the number of decimals be configurable from the definition as well? I think having a "0 decimals" which calls round might be a very common use case.
There was a problem hiding this comment.
Btw technically speaking, since we cannot really "script" from outside easily having label as string param is not useful since one can only provide hardcoded text that is why I've also added common use case of showing current value with 2 decimal places but I can also add number of decimal places as a param.
There was a problem hiding this comment.
having label as string param is not useful since one can only provide hardcoded text
@uberchilly I agree. I also think the current solution is not great for i18n. This maybe could be re-implemented with something similar to sprintf or a way for users to specify where in the string they want the number (to enable them to pass '## Birds' so the label is "18 Birds" rather than "Birds: 18") (but I don't think this is a blocker for the feature).
I've also added common use case of showing current value with 2 decimal places but I can also add number of decimal places as a param.
I don't think 2 decimal places is more common than 1 decimal place or 0 (or 5), it all depends on the number of "divisions" or the size of the increment. For example, if I were to use this widget with integer values (between 0 and 100 with a step of 10), I think I wouldn't want to see any decimal positions anywhere.
If I was doing between 0 to 1 with 1000 divisions, I'd certainly would want to see increments in the 3rd decimal.
Instead of attempting to figure out what's the ideal number of decimals, I'd just add a value so users can configure it (and that's why I suggested it).
There was a problem hiding this comment.
I agree in general, but on the other hand other already supported widgets have bigger limitations imo. In my personal project because of these limitations I had to also add DoubleText and IntText as local widgets because I couldn't do what was suggested here somewhere and that was to add string variant of value that I need to show in Text in data beside regular double for example, and showing number value in Text widget is much more common usecase than using label in slider. So, I am not sure how deep should each widget go in terms of supporting things vs an effort to add scripting easily to prepare values from outside
Description
This adds Slider widget to material widgets.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.