[rfw] Material slider widget #6610

uberchilly · 2024-04-25T12:42:54Z

Description

This adds Slider widget to material widgets.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

- Added tests

Hixie · 2024-05-07T03:15:39Z

packages/rfw/lib/src/flutter/material_widgets.dart

+      onChanged: source.handler(['onChanged'],
+              (HandlerTrigger trigger) => (double value) {
+            trigger({'value': value});
+          }),


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).

Suggested change

onChanged: source.handler(['onChanged'],

(HandlerTrigger trigger) => (double value) {

trigger({'value': value});

}),

onChanged: source.handler(['onChanged'],

(HandlerTrigger trigger) => (double value) {

trigger({'value': value});

},

),

(same below)

Hixie · 2024-05-07T03:16:37Z

packages/rfw/lib/src/flutter/material_widgets.dart

+    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);


should there be a space after the :?

Hixie · 2024-05-07T04:29:09Z

packages/rfw/test/utils.dart

@@ -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 {


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

Hixie · 2024-05-07T04:29:29Z

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

uberchilly · 2024-05-07T09:24:20Z

Please let me know if there is something else I can fix after latest changes

Hixie

looks great, thanks!

auto-submit · 2024-05-08T03:54:23Z

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.

Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

uberchilly · 2024-05-08T07:18:22Z

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?

Hixie · 2024-05-08T18:19:21Z

Oh I forgot it needed a second review. Let me post about it in the Discord. Sorry about that. Yay for the bots!

ditman

LGTM, small question about the configurability of the "label" /cc @Hixie

ditman · 2024-05-08T19:44:17Z

packages/rfw/lib/src/flutter/material_widgets.dart

+    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);


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.

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.

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).

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

stuartmorgan · 2024-06-04T19:50:53Z

From triage: @ditman / @Hixie Is this ready to land, or is there still design discussion that needs to be resolved?

ditman · 2024-06-05T00:01:24Z

@stuartmorgan I'm not opposed to this landing!

flutter/packages@11e192a...586faa6 2024-06-05 ditman@gmail.com [google_sign_in_web] Update button_tester to use web_only library. (flutter/packages#6868) 2024-06-05 engine-flutter-autoroll@skia.org Roll Flutter from c246ecd to 27e0656 (17 revisions) (flutter/packages#6875) 2024-06-05 15619084+vashworth@users.noreply.github.com [path_provider] Skip verifying sample file on macOS (flutter/packages#6874) 2024-06-05 joonas.kerttula@codemate.com [google_maps_flutter] Custom marker size improvements (flutter/packages#4055) 2024-06-05 uberchilly@gmail.com [rfw] Material slider widget (flutter/packages#6610) 2024-06-04 ditman@gmail.com [ci] Manual roll Flutter to c246ecd (84 revisions) + fixes (flutter/packages#6863) 2024-06-04 molchanovia.dev@gmail.com Correcting the typo of Flutter in projects (flutter/packages#6850) 2024-06-04 joonas.kerttula@codemate.com [google_maps_flutter] Custom marker size improvements - platform impls (flutter/packages#6826) 2024-06-04 lrn@google.com Avoid cumbersome formatter workaround (flutter/packages#6573) 2024-06-04 15619084+vashworth@users.noreply.github.com Clean Xcode project before analyzing and testing (flutter/packages#6842) 2024-06-03 37848204+RobinCombrink@users.noreply.github.com [pigeon] Kotlin/Java method overloading for the `setUp` method (flutter/packages#6843) 2024-06-03 40719830+Alex-Usmanov@users.noreply.github.com [url_launcher] Add support for setting show title on Chrome Custom Tabs (flutter/packages#6097) 2024-06-03 ditman@gmail.com Revert "Roll Flutter from c85fa6a to 7eebe29 (#6836)" (flutter/packages#6860) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

### Description This adds Slider widget to material widgets.

uberchilly added 2 commits April 25, 2024 14:12

- Added Material Slider widget

e7fe375

- Added tests

Updated CHANGELOG.md

70c0c4d

uberchilly requested a review from Hixie as a code owner April 25, 2024 12:42

github-actions bot added the p: rfw Remote Flutter Widgets label Apr 25, 2024

uberchilly added 4 commits April 25, 2024 15:13

Fixed tests for web

a0d3e44

Fixed formatting and end of line in utils

e36ff8f

Merge branch 'main' into material_slider

4174533

Merge branch 'main' into material_slider

77f6303

Hixie reviewed May 7, 2024

View reviewed changes

uberchilly added 3 commits May 7, 2024 09:39

- Fixed material slider handler indentations per guideline.

d502d46

- Changed slideToValue to be function instead of extension - Added space after : in material slider label default text

- Fixed unused import

771f595

- Fixed formatting for utils

6f89100

uberchilly added 2 commits May 7, 2024 13:32

Merge branch 'main' into material_slider

5b48d90

Merge branch 'main' into material_slider

905f8a9

Hixie approved these changes May 8, 2024

View reviewed changes

Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label May 8, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 8, 2024

ditman approved these changes May 8, 2024

View reviewed changes

Merge branch 'main' into material_slider

77028ca

Merge branch 'main' into material_slider

bb567e6

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 5, 2024

auto-submit bot merged commit 5183cac into flutter:main Jun 5, 2024
74 checks passed

uberchilly deleted the material_slider branch June 5, 2024 10:29

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Packages from 11e192a86db3 to 3885ac2c5c6c (10 revisions) flutter/flutter#149744

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2024

5183cac [rfw] Material slider widget (flutter/packages#6610)

521e0c0

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Packages from 11e192a86db3 to 67b082d4e89b (11 revisions) flutter/flutter#149751

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2024

5183cac [rfw] Material slider widget (flutter/packages#6610)

94fb0b5

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Packages from 11e192a86db3 to 023210f7bf59 (12 revisions) flutter/flutter#149760

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2024

5183cac [rfw] Material slider widget (flutter/packages#6610)

2555369

engine-flutter-autoroll mentioned this pull request Jun 6, 2024

Roll Packages from 11e192a86db3 to 586faa631499 (13 revisions) flutter/flutter#149820

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2024

5183cac [rfw] Material slider widget (flutter/packages#6610)

92c6a2e

arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024

[rfw] Material slider widget (flutter#6610)

16414ef

### Description This adds Slider widget to material widgets.

reidbaker mentioned this pull request Jul 2, 2024

i151188 update espresso dependencies v2 reidbaker/packages#6

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rfw] Material slider widget #6610

[rfw] Material slider widget #6610

uberchilly commented Apr 25, 2024

Hixie May 7, 2024

Hixie May 7, 2024

Hixie May 7, 2024

Hixie May 7, 2024

Hixie commented May 7, 2024

uberchilly commented May 7, 2024

Hixie left a comment

auto-submit bot commented May 8, 2024

uberchilly commented May 8, 2024

Hixie commented May 8, 2024

ditman left a comment

ditman May 8, 2024

uberchilly May 8, 2024 •

edited

Loading

ditman May 9, 2024 •

edited

Loading

uberchilly May 9, 2024

stuartmorgan commented Jun 4, 2024

ditman commented Jun 5, 2024

[rfw] Material slider widget #6610

[rfw] Material slider widget #6610

Conversation

uberchilly commented Apr 25, 2024

Description

Pre-launch Checklist

Hixie May 7, 2024

Choose a reason for hiding this comment

Hixie May 7, 2024

Choose a reason for hiding this comment

Hixie May 7, 2024

Choose a reason for hiding this comment

Hixie May 7, 2024

Choose a reason for hiding this comment

Hixie commented May 7, 2024

uberchilly commented May 7, 2024

Hixie left a comment

Choose a reason for hiding this comment

auto-submit bot commented May 8, 2024

uberchilly commented May 8, 2024

Hixie commented May 8, 2024

ditman left a comment

Choose a reason for hiding this comment

ditman May 8, 2024

Choose a reason for hiding this comment

uberchilly May 8, 2024 • edited Loading

Choose a reason for hiding this comment

ditman May 9, 2024 • edited Loading

Choose a reason for hiding this comment

uberchilly May 9, 2024

Choose a reason for hiding this comment

stuartmorgan commented Jun 4, 2024

ditman commented Jun 5, 2024

uberchilly May 8, 2024 •

edited

Loading

ditman May 9, 2024 •

edited

Loading