[Material] Update slider and slider theme with new sizes, shapes, and color mappings by clocksmith · Pull Request #30390 · flutter/flutter

clocksmith · 2019-04-02T15:31:48Z

Description

This PR makes a number of changes to the visual appearance of material sliders:

Sizes/Shapes
** enabled thumb radius from 6 to 10
** disabled thumb radius from 4 to 10 with no gap
** default track shape is a rounded rect rather than a rect
**
Colors
** all of the colors now use the new color scheme
** overlay opacity has been reduce from 16% to 12%
** value indicator text color now respects the indicator it is on by using onPrimary
** disabledThumb color no respects the surface it is on by using onSurface

The slider theme is also now constructed consistently with other theme objects within the ThemeData. By default, all values are null, and have default values that are resolved in the slider itself, rather than in the slider theme.

Related Issues

Closes #28706
Closes #30314
Closes #30315
Closes #30316
Closes #30317

Tests

No new tests were added, but many had to be updated to the new sizes and colors. Tests for default slider theme data property resolving were removed since the SliderThemeData no longer does that.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Yes, this is a breaking change (Please read Handling breaking changes).
https://groups.google.com/forum/#!topic/flutter-announce/w5TGYkN7Fe4
No, this is not a breaking change.

…er-breaking

rami-a

LGTM from as much as I know about sliders

rami-a · 2019-04-04T19:51:34Z

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

+    bool isEnabled,
+    bool isDiscrete,
+  }) {
+    isEnabled = isEnabled ?? false;


Can these 2 be set to false as default values in the param list instead of here?

rami-a · 2019-04-04T19:57:08Z

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

    bool isEnabled,
    bool isDiscrete,
  }) {
+    isEnabled = isEnabled ?? false;


Can these 2 be set to false as default values in the param list instead of here?

…er-breaking

willlarche · 2019-04-08T20:07:18Z

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

 }

-// The following shapes are the material defaults.
+/// Base track shape with default sizing.


Explain just a little more in the first line of the description.

willlarche · 2019-04-08T20:20:06Z

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

+///
+/// See also:
+///
+///  * [Slider] for the component that this is meant to display this shape.


Fixed this, and where it was in other places. Also updated these to follow the "See also" pattern so its more consistent throughout this file.

willlarche · 2019-04-08T20:21:45Z

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

+/// See also:
+///
+///  * [Slider] for the component that this is meant to display this shape.
+///  * [SliderThemeData] where an instance of this class is set to inform the


Capitalize the W in Where

Changed to adding a comma after the [Foo] to be consistent with the other "See also" sections

willlarche · 2019-04-08T20:23:50Z

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

+///    shapes.
+///  * [RectangularSliderTrackShape] for a similar track with blunt edges.
+class RoundedRectSliderTrackShape extends SliderTrackShape with BaseSliderTrackShape {
+  /// Create a slider track that draws 2 rectangles with rounded outer edges.


willlarche · 2019-04-08T20:26:00Z

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

+      return;
+    }
+
+    // Assign the track segment paints, which are left: active, right: inactive,


Are the left and right in RTL?

And I see one line below that they are.

Change this to leading and trailing instead of right and left and you get all that for free.

I mean the comments will be clear without having to say what happens in RTL.

It is noted that it is reversed for RTL on the next line

…er-breaking

willlarche

Thank you! It's a big one!!

willlarche · 2019-04-09T14:38:46Z

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

+      isDiscrete: isDiscrete,
+    );
+
+    final Rect leftTrackArcRect = Rect.fromLTWH(trackRect.left, trackRect.top, trackRect.height, trackRect.height);


Can you add a comment explaining why you have the height for both width and height/

willlarche · 2019-04-09T14:53:46Z

packages/flutter/test/material/slider_test.dart

    log.clear();
  });

+  // TODO(clocksmith): delete this test in favor of something in slider_theme_test


Is this stale?

Yes, thanks. Deleted.

willlarche · 2019-04-09T14:54:37Z

packages/flutter/test/material/slider_test.dart

      ),
    ));
-    expect(tester.renderObject<RenderBox>(find.byType(Slider)).size, const Size(144.0 + 2.0 * 16.0, 600.0));
+    expect(tester.renderObject<RenderBox>(find.byType(Slider)).size, const Size(144.0 + 2.0 * 24.0, 600.0));


Anywhere you're doing math, you can drop the .0s.

Keeping for consistency within file.

…er-breaking

HansMuller

Everything looks pretty good.

It would be useful to include a regression test that verifies (as best we can) that the new dimensions and appearance of a default enabled/disabled slider never changes. Although this could be done with a golden image test, a unit test would be better at pinpointing the source of regressions. I added similar tests for buttons recently: https://github.com/flutter/flutter/blob/master/packages/flutter/test/material/buttons_test.dart#L18

HansMuller · 2019-04-10T17:09:17Z

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

+    // colors come from the ThemeData.colorScheme. These colors, along with
+    // the default shapes and text styles are aligned to the Material
+    // Guidelines.
+    sliderTheme = sliderTheme.copyWith(


This is OK, since it's straightforward, although it's a bit of a monster. If we added a SliderTheme.merge() method that only changed null properties, then what's going on it might be a little clearer:

final SliderThemeData sliderTheme = SliderTheme.of( // Non-null widget properties override SliderTheme values. .copyWith( activeTrackColor: widget.activeColor, inactiveTrackColor: widget.inactiveColor, activeTickMarkColor: widget.inactiveColor, inactiveTickMarkColor: widget.activeColor, thumbColor: widget.activeColor, overlayColor: widget.activeColor?.withOpacity(0.12), valueIndicatorColor: widget.activeColor) // Default all of the remaining null SliderTheme properties. .merge( activeTrackColor: theme.colorScheme.primary, inactiveTrackColor: theme.colorScheme.primary.withOpacity(0.24), disabledActiveTrackColor: theme.colorScheme.onSurface.withOpacity(0.32), disabledInactiveTrackColor: theme.colorScheme.onSurface.withOpacity(0.12), activeTickMarkColor: theme.colorScheme.onPrimary.withOpacity(0.54), inactiveTickMarkColor: theme.colorScheme.primary.withOpacity(0.54), disabledActiveTickMarkColor: theme.colorScheme.onPrimary.withOpacity(0.12), disabledInactiveTickMarkColor: theme.colorScheme.onSurface.withOpacity(0.12), thumbColor: theme.colorScheme.primary, disabledThumbColor: theme.colorScheme.onSurface.withOpacity(0.38), overlayColor: theme.colorScheme.primary.withOpacity(0.12), valueIndicatorColor: theme.colorScheme.primary, trackShape: _defaultTrackShape, tickMarkShape: _defaultTickMarkShape, thumbShape: _defaultThumbShape, overlayShape: _defaultOverlayShape, valueIndicatorShape: _defaultValueIndicatorShape, showValueIndicator:_defaultShowValueIndicator, valueIndicatorTextStyle: theme.textTheme.body2.copyWith(color: theme.colorScheme.onPrimary), ); )

After some experimentation, I do not think the .merge method needed would be similar enough to the merge on TextTheme and TextStyle, so it might be confusing to have .merge methods in different objects that behave differently.

I agree that we haven't established a consistent meaning for "merge" methods and adding one here that works like ListTileTheme.merge but not like TextStyle.merge or Border.merge will not help matters.

We can solve the general problem and then update this monster in a separate PR.

HansMuller · 2019-04-10T17:37:35Z

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

+/// Base track shape that provides an implementation of [getPreferredRect] for
+/// default sizing.
+///
+/// The [SliderTrackShape]s that use this base class are:


This shouldn't be needed, DartDoc will generate a list of implementers. A "See also" section that named these subclasses and provided a one-liner explanation of what they mean would be useful.

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

HansMuller · 2019-04-10T17:44:39Z

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

+/// [parentBox], but is padded by the larger of [RoundSliderOverlayShape]
+/// radius and [RoundSliderThumbShape]. The height is defined by the
+/// [SliderThemeData.trackHeight]. The color is determined by the [Slider]'s
+/// enabled state and the track piece's active state which are defined by:


"track piece"?

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

HansMuller · 2019-04-10T17:51:45Z

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

+    bool isEnabled = false,
+    bool isDiscrete = false,
  }) {
+    final double thumbWidth = sliderTheme.thumbShape.getPreferredSize(isEnabled, isDiscrete).width;


assert sliderTheme != null etc. Here and elsewhere.

Done in all getPreferredFoo and paint methods

HansMuller · 2019-04-10T17:55:46Z

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

-  // TODO(clocksmith): This needs to be changed to 10 according to spec.
  const RoundSliderThumbShape({
-    this.enabledThumbRadius = 6.0,
+    this.enabledThumbRadius = 10.0,


The doc for the enabledThumbRadius property should admit that the default is 10

HansMuller · 2019-04-10T17:57:51Z

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

    final Offset bottomKnobStart = Offset(
      _bottomLobeRadius * math.cos(_bottomLobeStartAngle),
-      _bottomLobeRadius * math.sin(_bottomLobeStartAngle),
+      _bottomLobeRadius * math.sin(_bottomLobeStartAngle) - 2,


What's the "-2" doing here (it seems odd to be subtracting 2 from a radians value)? It's OK to say that it was derived "experimentally" so long as we explain what effect it has.

It is the y value of the offset, it was experimentally to get the bottom lobe in the right place after adjusting the lobe radius

…er-breaking

HansMuller

LGTM

HansMuller · 2019-04-18T17:44:18Z

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

+    // colors come from the ThemeData.colorScheme. These colors, along with
+    // the default shapes and text styles are aligned to the Material
+    // Guidelines.
+    sliderTheme = sliderTheme.copyWith(


I agree that we haven't established a consistent meaning for "merge" methods and adding one here that works like ListTileTheme.merge but not like TextStyle.merge or Border.merge will not help matters.

We can solve the general problem and then update this monster in a separate PR.

…er-breaking

…pes, and color mappings (#30390)" This reverts commit b1039f0.

…pes, and color mappings (#30390)" (#31339) This reverts commit b1039f0. (This is a temporary revert while some of our customers deal with the breaking changes involved.)

… color mappings (2nd attempt) (#31564) #30390 was rolled back. This PR will re-roll it forward. This PR makes a number of changes to the visual appearance of material sliders: Sizes/Shapes ** enabled thumb radius from 6 to 10 ** disabled thumb radius from 4 to 10 with no gap ** default track shape is a rounded rect rather than a rect ** Colors ** all of the colors now use the new color scheme ** overlay opacity has been reduce from 16% to 12% ** value indicator text color now respects the indicator it is on by using onPrimary ** disabledThumb color no respects the surface it is on by using onSurface The slider theme is also now constructed consistently with other theme objects within the ThemeData. By default, all values are null, and have default values that are resolved in the slider itself, rather than in the slider theme.

clocksmith added 9 commits March 18, 2019 12:26

first

ae83946

finished breaking changes, need test updates

33ea3a2

Merge branch 'master' of https://github.com/flutter/flutter into slid…

6905520

…er-breaking

m

7160df0

merge

e401055

slider_test tests passing

8130024

updating slider theme tests

8ffbaed

tests pass

b0498fd

Merge branch 'master' of https://github.com/flutter/flutter into slid…

3db99f1

…er-breaking

clocksmith requested review from johnsonmh, rami-a and willlarche April 2, 2019 15:31

googlebot added the cla: yes label Apr 2, 2019

HansMuller added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 2, 2019

clocksmith changed the title ~~[Material] Update slider and slider theme with new sizes, shapes, and color mappings~~ [WIP] [Material] Update slider and slider theme with new sizes, shapes, and color mappings Apr 2, 2019

clocksmith added 11 commits April 2, 2019 14:59

Merge branch 'master' of https://github.com/flutter/flutter into slid…

ac52a38

…er-breaking

m

c3828a9

fix lobe

6fc7be4

m

fb6c98a

fix lobe

253141b

remove unused import

7a48d4e

analuzer

4b7304b

analyzer

9cb0514

tests pass

76ba565

add docs

24f1d4f

remove whitespace

9def771

clocksmith changed the title ~~[WIP] [Material] Update slider and slider theme with new sizes, shapes, and color mappings~~ [Material] Update slider and slider theme with new sizes, shapes, and color mappings Apr 4, 2019

Empty commit to rerun tests

0ecb5f4

rami-a approved these changes Apr 4, 2019

View reviewed changes

clocksmith added 2 commits April 8, 2019 14:06

Merge branch 'master' of https://github.com/flutter/flutter into slid…

aa91414

…er-breaking

resolving comments

d93da77

willlarche reviewed Apr 8, 2019

View reviewed changes

clocksmith added 4 commits April 8, 2019 16:56

resolve comments

0f392bb

Merge branch 'master' of https://github.com/flutter/flutter into slid…

4679b45

…er-breaking

Merge branch 'master' of https://github.com/flutter/flutter into slid…

5f1a182

…er-breaking

resolve comments

a3911c6

willlarche approved these changes Apr 9, 2019

View reviewed changes

clocksmith added 4 commits April 9, 2019 13:11

resolve comments

1976859

Merge branch 'master' of https://github.com/flutter/flutter into slid…

ae34070

…er-breaking

Merge branch 'master' of https://github.com/flutter/flutter into slid…

7c65374

…er-breaking

remove invalid theme data test on slider

6441278

HansMuller reviewed Apr 10, 2019

View reviewed changes

clocksmith added 4 commits April 10, 2019 18:52

adding merge and in the middle of writing test

d4533f7

Merge branch 'master' of https://github.com/flutter/flutter into slid…

0d99bff

…er-breaking

undo sliderThemeData.merge additions

2ba2ea1

resolve comments

f783391

HansMuller approved these changes Apr 18, 2019

View reviewed changes

clocksmith added 2 commits April 18, 2019 13:51

Merge branch 'master' of https://github.com/flutter/flutter into slid…

f1f059c

…er-breaking

Merge branch 'master' of https://github.com/flutter/flutter into slid…

136a92a

…er-breaking

clocksmith merged commit b1039f0 into flutter:master Apr 18, 2019

johnsonmh added a commit that referenced this pull request Apr 20, 2019

Revert "[Material] Update slider and slider theme with new sizes, sha…

3ae7ec2

…pes, and color mappings (#30390)" This reverts commit b1039f0.

johnsonmh mentioned this pull request Apr 20, 2019

Revert "[Material] Update slider and slider theme with new sizes, shapes, and color mappings" #31339

Merged

clocksmith mentioned this pull request Apr 24, 2019

[Material] Update slider and slider theme with new sizes, shapes, and color mappings (2nd attempt) #31564

Merged

9 tasks

clocksmith deleted the slider-breaking branch September 12, 2019 16:55

xu-baolin mentioned this pull request Sep 4, 2020

Deprecated unused property [RectangularSliderTrackShape.disabledThumbGapWidth] #65246

Merged

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

Conversation

clocksmith commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

rami-a 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!

willlarche 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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

clocksmith commented Apr 2, 2019 •

edited

Loading