Slider Visual Update #14901

gspencergoog · 2018-02-27T00:30:12Z

This implements an update to the look of the Slider widget.

Specifically, it does the following:

Adds the ability to customize the colors of all components of the slider
Adds the ability to customize the shape of the slider thumb and value indicator
Adds the ability to show the value indicator on continuous sliders
Updates the default value indicator to be a "paddle" shape with new animations.
Changes the tick marks to be visible all the time on discrete sliders
Fixes a memory leak of an animation controller.
Removes "thumbOpenAtMin" flag, which is no longer needed, and can be emulated by the
custom thumb shape support. It was not widely used.

Adds tests for all of the new features.

HansMuller

This looks great! Most of my feedback is just minor edits

HansMuller · 2018-02-27T23:32:15Z

packages/flutter/lib/src/material/slider.dart

@@ -2,46 +2,55 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-import 'dart:math' as math;
+import 'dart:math';


Within flutter it's conventional to use math. for math package imports.

HansMuller · 2018-02-27T23:34:25Z

packages/flutter/lib/src/material/slider.dart

 /// that use a slider will listen for the [onChanged] callback and rebuild the
 /// slider with a new [value] to update the visual appearance of the slider.
 ///
 /// By default, a slider will be as wide as possible, centered vertically. When
-/// given unbounded constraints, it will attempt to make the track 144 pixels
+/// given unbounded constraints, it will attempt to make the rail 144 pixels


It would be helpful to insert a paragraph above this one that identifies the slider's parts (rail, thumb, value indicator, etc). It would be OK to just copy the material from SliderThemeData.

HansMuller · 2018-02-27T23:37:09Z

packages/flutter/lib/src/material/slider.dart

-    this.thumbOpenAtMin: false,
-  }) : assert(value != null),
-       assert(min != null),
+  }) : assert(value != null),       assert(min != null),


linebreak between the asserts

Whoa, I think that was just me accidentally hitting the backspace key...

HansMuller · 2018-02-27T23:41:56Z

packages/flutter/lib/src/material/slider.dart

  final String label;

-  /// The color to use for the portion of the slider that has been selected.
+  /// The color to use for the portion of the slider rail that has been
+  /// selected.


In this case "selected" means between the slider's min limit and its value, right? It might be better to just say that than to ask the user to understand what the selected part of the rail is.

Note also: it looks like we're referring to this same part of the rail as "active".

Yeah, I've been trying to unify the language to match the spec, but it's a little fragmented. Fixed.

HansMuller · 2018-02-27T23:45:37Z

packages/flutter/lib/src/material/slider.dart

@@ -132,30 +142,30 @@ class Slider extends StatefulWidget {
  /// A label to show above the slider when the slider is active.
  ///
  /// Typically used to display the value of a discrete slider.
+  ///
+  /// The label is rendered using the active [ThemeData]'s


Should you point out that the label is displayed within the value indicator, etc.?

Sure. It's not guaranteed to be within a custom indicator, but it is drawn by it.

HansMuller · 2018-02-28T02:08:54Z

packages/flutter/lib/src/material/slider_theme.dart

+  );
+}
+
+class DefaultSliderThumbShape extends SliderComponentShape {


Need Dartdoc here

HansMuller · 2018-02-28T02:09:52Z

packages/flutter/lib/src/material/slider_theme.dart

+  }
+}
+
+class DefaultSliderValueIndicatorShape extends SliderComponentShape {


See earlier comment about dartdoc, not calling this DefaultFoo

Changed to PaddleSliderValueIndicatorShape. Kind of awkward, I know, but descriptive.

HansMuller · 2018-02-28T02:13:19Z

packages/flutter/lib/src/material/theme_data.dart

@@ -306,6 +326,14 @@ class ThemeData {
  /// icons placed on top of the primary color (e.g. toolbar text).
  final Brightness primaryColorBrightness;

+  /// A lighter version of the primary color, used for showing subtle differences


Here and below: I think it would be enough to just say "A lighter/darker version of the primary color".

HansMuller · 2018-02-28T02:13:33Z

packages/flutter/lib/src/material/theme_data.dart

@@ -342,7 +370,7 @@ class ThemeData {
  ///
  ///  * [InkSplash.splashFactory], which defines the default splash.
  ///  * [InkRipple.splashFactory], which defines a splash that spreads out
-  ///    more aggresively than the default.
+  ///    more aggressively than the default.


HansMuller · 2018-02-28T02:15:06Z

packages/flutter/lib/src/material/theme_data.dart

@@ -418,6 +446,10 @@ class ThemeData {
  /// An icon theme that contrasts with the accent color.
  final IconThemeData accentIconTheme;

+  /// A slider theme that describes the colors and shapes used in


Maybe:

The colors and shapes used to render [Slider].

This is the default value of [SliderTheme.of].

It's not really a default value: it's the value, however it got set. But I like your idea, so I did something close.

HansMuller · 2018-03-01T21:00:41Z

LGTM

Hixie · 2018-03-02T01:47:15Z

This regressed our "undocumented members" benchmark by infinity percent. :-) (from 0 to 18.)

Hixie · 2018-03-05T00:07:20Z

packages/flutter/lib/src/material/theme_data.dart

+    description.add(new DiagnosticsProperty<IconThemeData>('iconTheme', iconTheme));
+    description.add(new DiagnosticsProperty<IconThemeData>('primaryIconTheme', primaryIconTheme));
+    description.add(new DiagnosticsProperty<IconThemeData>('accentIconTheme', accentIconTheme));
+    description.add(new DiagnosticsProperty<SliderThemeData>('sliderTheme', sliderTheme));


we should probably mark most of these such that they don't show up in the debugDumpApp() output, because it basically makes the output unreadable as it stands today.

(also, minor nit: this would be more readable if all the lines were consistently wrapped or not wrapped)

OK, in another PR I've reformatted these with dartfmt, but with a line length of 200. That seems to keep the line breaks at bay.

i was more worried about the problem of this all making debugDumpApp useless :-)

Yeah, I was wondering about that too. How does one mark them so they don't show up there? I didn't see a switch on the DiagnosticsProperty object. I added these because the previous toString was useless for debugging one of my tests.

Hixie · 2018-03-05T00:08:23Z

packages/flutter/lib/src/material/theme_data.dart

@@ -732,7 +846,8 @@ class _IdentityThemeDataCacheKey {
    // We are explicitly ignoring the possibility that the types might not
    // match in the interests of speed.
    final _IdentityThemeDataCacheKey otherKey = other;
-    return identical(baseTheme, otherKey.baseTheme) && identical(localTextGeometry, otherKey.localTextGeometry);
+    return identical(baseTheme, otherKey.baseTheme) &&
+        identical(localTextGeometry, otherKey.localTextGeometry);


does this lack of vertical alignment comes from dartfmt, perchance?

Yes, it is indenting a continued line by 4 spaces. Changing the line length to 200 seems to have solved that problem.

fwiw, i don't mind it being wrapped, i just want it to be more readable by keeping the vertical alignment consistent if it's wrapped

Hixie · 2018-03-05T00:08:39Z

packages/flutter/lib/src/material/typography.dart

+        new DiagnosticsProperty<TextStyle>('caption', caption, defaultValue: defaultTheme.caption));
+    description.add(
+        new DiagnosticsProperty<TextStyle>('button', button, defaultValue: defaultTheme.button));
+  }


this wrapping is also pretty hard to read

Also fixed in my other PR. #15078

Hixie · 2018-03-06T20:20:53Z

packages/flutter/test/material/slider_test.dart

+    // Test setting only the activeColor.
+    await tester.pumpWidget(buildApp(activeColor: customColor1));
+    expect(
+        sliderBox, paints..rect(color: customColor1)..rect(color: sliderTheme.inactiveRailColor));


here's another example of dartfmt breaking you

Hixie · 2018-03-06T20:21:36Z

packages/flutter/lib/src/widgets/icon_theme_data.dart

+    super.debugFillProperties(description);
+    if (color == null && _opacity == null && size == null) {
+      return;
+    }


we should always add the properties, just give them default values in the description so they get hidden by default in the output

OK, I set them all to have default values of 'null'.

Hixie · 2018-03-06T20:21:52Z

packages/flutter/lib/src/widgets/icon_theme_data.dart

-      opacity: opacity ?? this.opacity,
-      size: size ?? this.size
-    );
+        color: color ?? this.color, opacity: opacity ?? this.opacity, size: size ?? this.size);


dartfmt broke you here too

Yep. Fixed.

Hixie · 2018-03-06T20:23:40Z

packages/flutter/lib/src/material/theme_data.dart

-        accentIconTheme,
-        platform,
-      )
+        brightness,


looks like dartfmt broke you here too

Hixie · 2018-03-06T20:25:10Z

packages/flutter/lib/src/widgets/icon_theme_data.dart

-      opacity: other.opacity,
-      size: other.size
-    );
+    return copyWith(color: other.color, opacity: other.opacity, size: other.size);


Hixie · 2018-03-06T20:25:36Z

packages/flutter/test/material/slider_test.dart

+      final TestGesture gesture = await tester.startGesture(center);
+      await tester.pump();
+      await tester
+          .pump(const Duration(milliseconds: 500)); // wait for value indicator animation to finish.


Hixie · 2018-03-06T20:25:39Z

packages/flutter/test/material/slider_test.dart

+    }
+
+    Future<Null> expectValueIndicator(
+        {bool isVisible, SliderThemeData theme, int divisions, bool enabled: true}) async {


Hixie · 2018-03-06T20:25:55Z

packages/flutter/test/material/slider_test.dart

+                child: new Theme(
+                  data: Theme.of(context).copyWith(
+                      sliderTheme:
+                          Theme.of(context).sliderTheme.copyWith(showValueIndicator: show)),


this looks a bit weird

Hixie · 2018-03-06T20:26:01Z

packages/flutter/test/material/slider_test.dart

    final Key sliderKey = new UniqueKey();
    double value = 0.0;

-    Widget buildSlider({ double textScaleFactor }) {
+    Widget buildSlider(


Hixie · 2018-03-06T20:26:31Z

packages/flutter/test/material/slider_test.dart

+    gesture = await tester.startGesture(center);
+    await tester.pump();
+    await tester
+        .pump(const Duration(milliseconds: 500)); // wait for value indicator animation to finish.


several times in this test the comment caused dartfmt to wrap a "pump" call inconsistently with the rest of the code

Hixie · 2018-03-06T20:30:46Z

packages/flutter/lib/src/material/slider.dart

+    final ColorTween activeTickMarkEnableColor = new ColorTween(
+        begin: _sliderTheme.disabledActiveTickMarkColor, end: _sliderTheme.activeTickMarkColor);
+    final ColorTween inactiveTickMarkEnableColor = new ColorTween(
+        begin: _sliderTheme.disabledInactiveTickMarkColor, end: _sliderTheme.inactiveTickMarkColor);


these aren't really wrapped in the flutter style either, though here i don't think it impacts readabilty much

Hixie · 2018-03-06T20:35:07Z

packages/flutter/lib/src/material/slider_theme.dart

+  /// Defaults to the ambient [ThemeData.sliderTheme] if there is no
+  /// [SliderTheme] in the given build context.
+  ///
+  /// Typical usage is as follows:


sample code should be in a ## Sample code section.

Done (in another PR)

Hixie · 2018-03-06T20:44:26Z

packages/flutter/lib/src/material/slider_theme.dart

+  /// defaults when assigning them to the slider theme component colors.
+  ///
+  /// This is used to generate the default slider theme for a [ThemeData].
+  factory SliderThemeData.materialDefaults({


i would probably call this SliderThemeData.fromPrimaryColors. The name SliderThemeData.materialDefaults sounds like a constant that has material defaults, not a method that computes values from other values.

Hmm. Ok, but the reason I called it that was because the opacities that it overrides the given colors with, and the shapes it assigns for the components are all from the Material spec. This is also the method used to create the default theme for the Material ThemeData.

being from the material spec is implied, this is the material library. :-)

Hixie · 2018-03-06T20:44:52Z

packages/flutter/lib/src/material/slider_theme.dart

+      valueIndicatorColor: Color.lerp(a.valueIndicatorColor, b.valueIndicatorColor, t),
+      thumbShape: t < 0.5 ? a.thumbShape : b.thumbShape,
+      valueIndicatorShape: t < 0.5 ? a.valueIndicatorShape : b.valueIndicatorShape,
+      showValueIndicator: t < 0.5 ? a.showValueIndicator : b.showValueIndicator,


aw, the shapes can't lerp? :-( :-P

Sadly no. They encompass a lot of behaviors that would have made lerping complex, so I decided to cut my losses and eliminate the lerpTo and lerpFrom. Not that we can't add them, or add a subclass of SliderComponentShape that has them.

Hixie · 2018-03-06T20:45:30Z

packages/flutter/lib/src/material/slider_theme.dart

+
+  @override
+  bool operator ==(Object other) {
+    if (other.runtimeType != runtimeType) {


you might want to do an identical() test here first since there's so many subproperties and they're so often going to be identical.

Hixie · 2018-03-06T20:46:03Z

packages/flutter/lib/src/material/slider_theme.dart

+    description.add(new DiagnosticsProperty<ShowValueIndicator>(
+        'showValueIndicator', showValueIndicator,
+        defaultValue: defaultData.showValueIndicator));
+  }


these are inconsistently wrapped which is weird

I've already fixed this in my other PR.

This implements an update to the look of the Slider widget. Specifically, it does the following: * Adds the ability to customize the colors of all components of the slider * Adds the ability to customize the shape of the slider thumb and value indicator * Adds the ability to show the value indicator on continuous sliders * Updates the default value indicator to be a "paddle" shape with new animations. * Changes the tick marks to be visible all the time on discrete sliders * Fixes a memory leak of an animation controller. * Removes "thumbOpenAtMin" flag, which is no longer needed, and can be emulated by the custom thumb shape support. It was not widely used. * Adds tests for all of the new features.

googlebot added the cla: yes label Feb 27, 2018

gspencergoog requested a review from HansMuller February 27, 2018 00:30

gspencergoog force-pushed the m2_slider branch from 3dd695a to e3958f2 Compare February 27, 2018 00:50

HansMuller reviewed Feb 28, 2018

View reviewed changes

gspencergoog added 4 commits February 28, 2018 14:43

Convert theme data into Diagnosticable output.

532b76d

Initial pass for M2 slider

a101e35

Adding more tests

2c9db44

Making tests pass

3c051bf

gspencergoog force-pushed the m2_slider branch 3 times, most recently from b9fc3b2 to eba21b5 Compare March 1, 2018 20:05

Review Changes

d1cb3bc

gspencergoog force-pushed the m2_slider branch from eba21b5 to d1cb3bc Compare March 1, 2018 20:05

gspencergoog merged commit 701eff4 into flutter:master Mar 1, 2018

Hixie reviewed Mar 5, 2018

View reviewed changes

Hixie reviewed Mar 6, 2018

View reviewed changes

packages/flutter/lib/src/material/theme_data.dart

accentIconTheme,

platform,

)

brightness,

Copy link

Contributor

Hixie Mar 6, 2018

Choose a reason for hiding this comment

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

looks like dartfmt broke you here too

Hixie reviewed Mar 6, 2018

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021

Slider Visual Update #14901

Slider Visual Update #14901

Uh oh!

Conversation

gspencergoog commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller commented Mar 1, 2018

Uh oh!

Hixie commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Feb 27, 2018 •

edited

Loading