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.

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

New Button Universe #59702

Merged
merged 17 commits into from
Jul 9, 2020
Merged

Conversation

HansMuller
Copy link
Contributor

@HansMuller HansMuller commented Jun 17, 2020

Warning

The widget and theme called ContainedButton are being changed to ElevatedButton. See #61262.

Overview

Adds a new set of button widgets and themes that address the problems outlined in flutter.dev/go/material-button-system-updates. The overall goal is to make buttons more flexible, and easier to configure via constructor parameters or themes.

Demo

If you'd like to skip the explanation and jump right to the visuals or the source code,
there's a prototype DartPad demo. The source code is there (and all in one file), and the Flutter framework part is hosted here: https://github.com/HansMuller/flutter_buttons. It's (now) somewhat out of date wrt this PR. However, if you want interactively experiment with changes, doing so from within the DartPad demo may be useful.

Summary: Updating the Material Buttons and their Themes

Rather than try and evolve the existing button classes and their theme in-place, this PR introduces new replacement button widgets and themes. In addition to freeing us from the backwards compatibility labyrinthe that evolving the existing classes in-place would entail, the new names sync Flutter back up with the Material Design spec, which uses the new names for the button components.

Old Widget Old Theme New Widget New Theme
FlatButton ButtonTheme TextButton TextButtonTheme
RaisedButton ButtonTheme ContainedButton ContainedButtonTheme
OutlineButton ButtonTheme OutlinedButton OutlinedButtonTheme

The new themes follow the "normalized" pattern that Flutter adopted for new Material widgets about a year ago. Theme properties and widget constructor parameters are null by default. Non-null theme properties and widget parameters specify an override of the component's default value. Implementing and documenting default values is the sole responsibility of the button component widgets. The defaults themselves are based primarily on the overall Theme's colorScheme and textTheme.

ButtonStyle

We've added a new class called ButtonStyle which aggregates the buttons' visual properties. Most of ButtonStyle's properties are defined with MaterialStateProperty, so that they can represent different values for different button states.

Each of the new button widget classes has a static styleFrom() method that returns a ButtonStyle. The styleFrom method's parameters are simple values (not MaterialStateProperties) that include overrides for the ColorScheme colors that the button's style depends on. The styleFrom() method computes all of the dependent colors for all of the button's states.

Using the Theme to override a Button property like textColor

An app that uses TextButton and ContainedButton (nee FlatButton and RaisedButton) can configure the text color for all buttons by specifying a textButtonTheme and containedButtonTheme in the app's overall theme.

MaterialApp(
  theme: ThemeData(
    textButtonTheme: TextButtonThemeData(
      style: TextButton.styleFrom(primary: Colors.green)
    ),
    containedButtonTheme: ContainedButtonThemeData(
      style: ContainedButton.styleFrom(onPrimary: Colors.green)
    ),
  ),
  // ...
)

The TextButton's text is rendered in the ColorScheme's primary color by default, and the ContainedButton's text is rendered with the onPrimary color. We've created a new ButtonStyle for each of the corresponding themes that effectively overrides the text color. ContainedButtons use the primary color as background, so one would probably want to set that as well:

MaterialApp(
  theme: ThemeData(
    textButtonTheme: TextButtonThemeData(
      style: TextButton.styleFrom(primary: Colors.green)
    ),
    containedButtonTheme: ContainedButtonThemeData(
      style: ContainedButton.styleFrom(
        primary: Colors.yellow,
        onPrimary: Colors.green,
      )
    ),
  ),
  // ...
)

In both cases this approach creates a ButtonStyle where the other colors that depend on the primary or onPrimary colors have been updated as well. For example the highlight color that's shown when the button is tapped or focused is also included in the ButtonStyle because it depends on the primary color too.

This is usually what you want. However there are times when an app needs to more precisely control its buttons' appearance. For example, you might really want to only change the ContainedButton's text color, and for all possible states (focused, pressed, disabled, etc). To do that, create a ButtonStyle that specifies textColor:

MaterialApp(
  theme: ThemeData(
    containedButtonTheme: ContainedButtonThemeData(
      style: ButtonStyle(
        textColor: MaterialStateProperty.resolveWith<Color>(
          (Set<MaterialState> states) => Colors.green,
        ),
      ),
    ),
  ),
  // ...
)

The ButtonStyle's textColor is a MaterialProperty<Color> and in this case the property just maps all possible states to green. To only override the button's text color when the button was enabled:

MaterialApp(
  theme: ThemeData(
    containedButtonTheme: ContainedButtonThemeData(
      style: ButtonStyle(
        textColor: MaterialStateProperty.resolveWith<Color>(
          (Set<MaterialState> states) {
            return (states.contains(MaterialState.disabled)) ? null : Colors.green;
          },
        ),
      ),
    ),
  ),
  // ...
)

The MaterialStateProperty that we've created for the text color returns null when its button is disabled. That means that the component will use the default: either the widget's ButtonStyle parameter, or, if that's null too, then the widget's internal default.

Using the Theme to override Button shapes

ButtonStyle objects allow one to override all of the visual properties including the buttons' shapes. As noted below, one can give all of an app's buttons the "stadium" shape like this.

MaterialApp(
  home: Home(),
  theme: ThemeData(
    textButtonTheme: TextButtonThemeData(
      style: TextButton.styleFrom(
        shape: StadiumBorder(),
      ),
    ),
    containedButtonTheme: ContainedButtonThemeData(
      style: ContainedButton.styleFrom(
        shape: StadiumBorder(),
      ),
    ),
    outlinedButtonTheme: OutlinedButtonThemeData(
      style: OutlinedButton.styleFrom(
        shape: StadiumBorder(),
      ),
    ),
  ),
)

Using ButtonStyle to change the appearance of individual buttons

A ButtonStyle can also be applied to individual buttons. For example, to create an AlertDialog with "stadium" shaped action buttons, rather than wrapping the dialog's contents in a theme, one could just specify the same style for both buttons.

ButtonStyle style = OutlinedButton.styleFrom(shape: StadiumBorder());
showDialog(
  context: context,
  builder: (BuildContext context) {
    return AlertDialog(
      title: Text('AlertDialog Title'),
      content: Text('Stadium shaped action buttons, default outline'),
      actions: <Widget>[
        OutlinedButton(
          style: style,
          onPressed: () { dismissDialog(); },
          child: Text('Approve'),
        ),
        OutlinedButton(
          style: style,
          onPressed: () { dismissDialog(); },
          child: Text('Really Approve'),
        ),
      ],
    );
  },
);

Screen Shot 2020-05-11 at 11 31 04 AM

In this case, just like the others, the style only overrides the button shapes, all of of the other properties get their context-specific defaults in the usual way. To give one of the buttons a heavier primary colored outline, instead of the default thin gray outline:

showDialog(
  context: context,
  builder: (BuildContext context) {
    return AlertDialog(
      title: Text('AlertDialog Title'),
      content: Text('One Stadium shaped action button, with a heavy, primary color outline.',
      actions: <Widget>[
        OutlinedButton(
          style: OutlinedButton.styleFrom(shape: StadiumBorder()),
          onPressed: () { dismissDialog(); },
          child: Text('Approve'),
        ),
        OutlinedButton(
          style: OutlinedButton.styleFrom(
            shape: StadiumBorder(),
            side: BorderSide(
              width: 2,
              color: Theme.of(context).colorScheme.primary,
            ),
          ),
          onPressed: () { dismissDialog(); },
          child: Text('Really Approve'),
        ),
      ],
    );
  },
);

Screen Shot 2020-05-11 at 11 33 19 AM

Most of the button visual properties are specified in terms of MaterialStateProperty, which means that the property can have different values depending on its button's state. Using the convenient static styleFrom methods delegates creating the MaterialStateProperty values to the button class. It's easy enough to create them directly, to construct ButtonStyles with state-specific values. For example, to set up the second dialog buttons so that it only shows the heavier primary colored outline when it's hovered or focused:

OutlinedButton(
  style: OutlinedButton.styleFrom(
    shape: StadiumBorder(),
  ).copyWith(
    side: MaterialStateProperty.resolveWith<BorderSide>((Set<MaterialState> states) {
      if (states.contains(MaterialState.hovered) || states.contains(MaterialState.focused)) {
        return BorderSide(
          width: 2,
          color: Theme.of(context).colorScheme.primary,
        );
      }
      return null; // defer to the default
    },
  )),
  onPressed: () { dismissDialog(); },
  child: Text('Really Approve'),
),

In this case we've used the resolveWith() utility method to create a MaterialStateProperty that only overrides the default outline appearance when the button is either focused or hovered.

Fixes #54776.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 17, 2020
@HansMuller HansMuller force-pushed the new_button_universe branch 2 times, most recently from c19002a to 1a246b3 Compare June 23, 2020 02:04
@LasseRosenow
Copy link
Contributor

Will the old button widgets be annotated as deprecated to help us migrate?

@HansMuller
Copy link
Contributor Author

@lazylazyllama - yes, the old button classes will be deprecated, although not immediately. Before that happens we'll be updating API doc examples and demos, and etc.

Copy link
Member

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

This is an incredible change and should make buttons much easier to use and theme!!

Also love the thorough testing :)

final MaterialStateProperty<Color> foregroundColor;

/// The highlight color that's typically used to indicate that
/// the button is focused or hovered, pressed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// the button is focused or hovered, pressed.
/// the button is focused, hovered, or pressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}
}

class _LerpTextStyles implements MaterialStateProperty<TextStyle> {
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense for these MaterialStateProperty lerp classes to be their own public API? Perhaps down the line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might, we'll have to see if the pattern crops up again.

canRequestFocus: widget.enabled,
onFocusChange: _handleFocusedChanged,
autofocus: widget.autofocus,
splashFactory: InkRipple.splashFactory,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be read from the themeData.splashFactory instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We're going to need to provide a way for developers to simply configure a theme based on a ColorScheme that uses the 2018 text styles and the InkRipple splashFactory.

/// buttons in a subtree can be overridden with the
/// [ContainedButtonTheme], and the style of all of the contained
/// buttons in an app can be overridden with the [Theme]'s
/// [ThemeData.textButtonTheme] property.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// [ThemeData.textButtonTheme] property.
/// [ThemeData.containedButtonTheme] property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// See also:
///
/// * [TextButton], a simple flat button without a shadow.
/// * [OultinedButton], a [TextButton] with a border outline.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// * [OultinedButton], a [TextButton] with a border outline.
/// * [OutlinedButton], a [TextButton] with a border outline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// Create a text button from a pair of widgets that serve as the button's
/// [icon] and [label].
///
/// The icon and label are arranged in a row and padded by 12 logical pixels
Copy link
Member

Choose a reason for hiding this comment

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

I believe for TextButtons with Icons, this padding is incorrect. I think it should be 8 leading/trailing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, will fix that.

expect(tester.getSize(find.byKey(key1)), const Size(88.0, 48.0));

final Key key2 = UniqueKey();
await tester.pumpWidget(
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated from above with only materialTapTargetSize changed, could be extracted into an inner function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.

expect(inkFeatures, paints..rect(color: focusColor));
});

testWidgets('Does OutlinedButton work with autofocus', (WidgetTester tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't see a corresponding test like this in contained button tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added one.

import '../rendering/mock_canvas.dart';
import '../widgets/semantics_tester.dart';

void main() {
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a test for all 3 buttons that confirms the behavior of:

foo.style ?? theme.style ?? defaultStyle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added basic tests for all three new themes.

expect(tester.getSize(find.byKey(key1)), const Size(66.0, 48.0));

final Key key2 = UniqueKey();
await tester.pumpWidget(
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as ContainedButton's test, could put this into an inner function to avoid duplicating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done.

@flutter-github-sync
Copy link

Started Google testing for this PR

@flutter-github-sync
Copy link

Google testing failed...

@flutter-github-sync
Copy link

Started Google testing for this PR

@flutter-github-sync
Copy link

Google testing failed...

@pennzht pennzht self-requested a review June 25, 2020 09:29
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

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

Some minor nits and questions, but otherwise awesome! LGTM.

packages/flutter/lib/src/material/button_style.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/button_style.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/button_style_button.dart Outdated Show resolved Hide resolved
/// style of the [TextButton] subclass can be overidden with its
/// [TextButton.style] constructor parameter, or with a
/// [TextButtonTheme].
///
Copy link
Contributor

Choose a reason for hiding this comment

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

So is the difference between defaultStyleOf and themeStyleOf that defaultStyleOf provides the default style for a given subclass, where the themeStyleOf can override the defaults for a given instance of the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

packages/flutter/lib/src/material/button_style_button.dart Outdated Show resolved Hide resolved
Copy link
Member

@pennzht pennzht left a comment

Choose a reason for hiding this comment

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

Hi Hans,
Thank you very much for posting this PR! I really appreciate it.

Comment on lines +359 to +363
mouseCursor: t < 0.5 ? a.mouseCursor : b.mouseCursor,
visualDensity: t < 0.5 ? a.visualDensity : b.visualDensity,
tapTargetSize: t < 0.5 ? a.tapTargetSize : b.tapTargetSize,
animationDuration: t < 0.5 ? a.animationDuration : b.animationDuration,
enableFeedback: t < 0.5 ? a.enableFeedback : b.enableFeedback,
Copy link
Member

Choose a reason for hiding this comment

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

      mouseCursor: t < 0.5 ? a.mouseCursor : b.mouseCursor,

Should we make a function for this pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that it's very common, however I'm not sure a function call would make it easier to read. If we were going to use a function for this kind of lerp statement, we'd have to apply it across the whole framework. That would be a different PR.

assert(geometry1x != null);
assert(geometry2x != null);
assert(geometry3x != null);
assert(textScaleFactor != null && textScaleFactor >= 0);
Copy link
Member

Choose a reason for hiding this comment

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

Is there any case where textScaleFactor can be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. The documentation doesn't specify any limits: https://api.flutter.dev/flutter/widgets/MediaQueryData/textScaleFactor.html

The code handles all possible values, so there's no need to assert that its >= 0 here.

/// ContainedButton(
/// style: TextButton.styleFrom(primary: Colors.green),
/// )
///```
Copy link
Member

Choose a reason for hiding this comment

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

Should it be /// ``` (with a space)? (Same for a few other occurrences in this PR)

https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/foundation/diagnostics.dart#L410

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point.

Comment on lines 270 to 271
final VisualDensity resolvedVisualDensity = widgetStyle?.visualDensity ?? defaultStyle.visualDensity;
final MaterialTapTargetSize resolvedTapTargetSize = widgetStyle?.tapTargetSize ?? defaultStyle.tapTargetSize;
Copy link
Member

Choose a reason for hiding this comment

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

themeStyle is omitted. Is this because defaultStyle uses the same visualDensity and tapTargetSize as themeStyle?
https://github.com/flutter/flutter/pull/59702/files#diff-d5a6a7e71470fb01469989a4553e89adR250

Could themeStyle.visualDensity and defaultStyle.visualDensity diverge in the future? If they might, it make sense to use widgetStyle?.visualDensity ?? themeStyle?.visualDensity ?? defaultStyle.visualDensity here, so that later we don't forget it.

Copy link
Member

Choose a reason for hiding this comment

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

Using the suggestion above, this becomes:

        final VisualDensity resolvedVisualDensity = effectiveValue((ButtonStyle style) => style?.visualDensity);
        final MaterialTapTargetSize resolvedTapTargetSize = effectiveValue((ButtonStyle style) => style?.tapTargetSize);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! That was a mistake. It also implies that I need more tests.

@override
Widget build(BuildContext context) {
final double scale = MediaQuery.of(context, nullOk: true)?.textScaleFactor ?? 1;
final double gap = scale <= 1 ? 8 : lerpDouble(8, 4, math.min(scale - 1, 1));
Copy link
Member

Choose a reason for hiding this comment

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

I think the following form is clearer (same for the other two final double gaps):

    final double gap = lerpDouble(8, 4, (scale - 1).clamp(0, 1));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, in fact I'd written it that way. I changed it because the analyzer complained and I wasn't able to find a nice way around the complaint:

  error • The argument type 'num' can't be assigned to the parameter type 'double' •
         packages/flutter/lib/src/material/contained_button.dart:419:41 • argument_type_not_assignable

I believe it's complaining because clamp returns a num, not a double. Introducing another line of code didn't seem to make the statement simpler either, so ...

Comment on lines +106 to +121
final ButtonStyle style = ButtonStyle(
textStyle: textStyle,
backgroundColor: backgroundColor,
foregroundColor: foregroundColor,
overlayColor: overlayColor,
elevation: elevation,
padding: padding,
minimumSize: minimumSize,
side: side,
shape: shape,
mouseCursor: mouseCursor,
visualDensity: visualDensity,
tapTargetSize: tapTargetSize,
animationDuration: animationDuration,
enableFeedback: enableFeedback,
);
Copy link
Member

Choose a reason for hiding this comment

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

We also need to test copying with / merging two different ButtonStyles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a few additional copyWith/merge checks.

Comment on lines 367 to 386
static MaterialStateProperty<TextStyle> _lerpTextStyles(MaterialStateProperty<TextStyle> a, MaterialStateProperty<TextStyle> b, double t) {
if (a == null && b == null)
return null;
return _LerpTextStyles(a, b, t);
}

static MaterialStateProperty<Color> _lerpColors(MaterialStateProperty<Color> a, MaterialStateProperty<Color> b, double t) {
if (a == null && b == null)
return null;
return _LerpColors(a, b, t);
}

static MaterialStateProperty<double> _lerpDoubles(MaterialStateProperty<double> a, MaterialStateProperty<double> b, double t) {
if (a == null && b == null)
return null;
return _LerpDoubles(a, b, t);
}

static MaterialStateProperty<EdgeInsetsGeometry> _lerpInsets(MaterialStateProperty<EdgeInsetsGeometry> a, MaterialStateProperty<EdgeInsetsGeometry> b, double t) {
if (a == null && b == null)
return null;
return _LerpInsets(a, b, t);
}

static MaterialStateProperty<Size> _lerpSizes(MaterialStateProperty<Size> a, MaterialStateProperty<Size> b, double t) {
if (a == null && b == null)
return null;
return _LerpSizes(a, b, t);
}

static MaterialStateProperty<BorderSide> _lerpSides(MaterialStateProperty<BorderSide> a, MaterialStateProperty<BorderSide> b, double t) {
if (a == null && b == null)
return null;
return _LerpSides(a, b, t);
}

static MaterialStateProperty<OutlinedBorder> _lerpShapes(MaterialStateProperty<OutlinedBorder> a, MaterialStateProperty<OutlinedBorder> b, double t) {
if (a == null && b == null)
return null;
return _LerpShapes(a, b, t);
}
Copy link
Member

Choose a reason for hiding this comment

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

I understand that it's necessary.

In the long term, I think it makes sense to introduce, say, a Lerpable mixin, so that we don't have to write duplicate code. Such a mixin can also handle null on its own.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code per MH's suggestion, #59702 (comment)

It's not a mixin, but it does eliminate much of the boilerplate.

Comment on lines 244 to 255
T resolve<T>(MaterialStateProperty<T> Function(ButtonStyle style) getProperty) {
final MaterialStateProperty<T> widgetValue = getProperty(widgetStyle);
final MaterialStateProperty<T> themeValue = getProperty(themeStyle);
final MaterialStateProperty<T> defaultValue = getProperty(defaultStyle);
assert(defaultValue != null);
return widgetValue?.resolve(_states) ?? themeValue?.resolve(_states) ?? defaultValue.resolve(_states);
}
Copy link
Member

Choose a reason for hiding this comment

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

The "widget ?? theme ?? default" pattern shows up a bit often (see below), so you can use the following form to reduce some duplicate code:

    T effectiveValue<T>(T Function(ButtonStyle style) getProperty) {
      final T widgetValue  = getProperty(widgetStyle);
      final T themeValue   = getProperty(themeStyle);
      final T defaultValue = getProperty(defaultStyle);
      return widgetValue ?? themeValue ?? defaultValue;
    }

    T resolve<T>(MaterialStateProperty<T> Function(ButtonStyle style) getProperty) {
      final T resolvedValue = effectiveValue(
        (ButtonStyle style) => getProperty(style)?.resolve(_states),
      );
      assert (resolvedValue != null);
      return resolvedValue;
    }

There is a slight difference, though: in your original version, we assert getProperty(defaultStyle) is non-null, but the final result could still be null (if defaultValue is a non-null object that always resolves to null). Here, we assert that the final value returned is non-null.
You can still edit this code to suit the requirements.

Copy link
Member

Choose a reason for hiding this comment

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

Also, there's an extra space on L246 but the three ='s are not aligned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a nice way to factor the code, I've used it. The new version is even more closure-intense than the old one, hopefully the compiler will see how all of that can be flattened out.

// all of the states that buttons care about except MaterialState.disabled.
final MaterialStateProperty<Color> overlayColor = MaterialStateProperty.resolveWith<Color>(
(Set<MaterialState> states) {
return widgetStyle?.overlayColor?.resolve(states) ?? themeStyle?.overlayColor?.resolve(states) ?? defaultStyle.overlayColor?.resolve(states);
Copy link
Member

Choose a reason for hiding this comment

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

Using the suggestion above, this becomes:

        return effectiveValue((ButtonStyle style) => style?.overlayColor?.resolve(states));

}
}

class _LerpColors implements MaterialStateProperty<Color> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think something along these lines could work to save a few lines of code?

class _LerpProperties<T> implements MaterialStateProperty<T> {
  const _LerpProperties(this.a, this.b, this.t, this.lerpFunction);

  final MaterialStateProperty<T> a;
  final MaterialStateProperty<T> b;
  final double t;
  final T Function(T, T, double) lerpFunction;

  @override
  T resolve(Set<MaterialState> states) {
    final T resolvedA = a?.resolve(states);
    final T resolvedB = b?.resolve(states);
    return lerpFunction(resolvedA, resolvedB, t);
  }
}

Then the calls could look like:

static MaterialStateProperty<TextStyle> _lerpTextStyles(MaterialStateProperty<TextStyle> a, 
  MaterialStateProperty<TextStyle> b, double t) {
    if (a == null && b == null)
      return null;
    return _LerpProperties<TextStyle>(a, b, t, TextStyle.lerp);
  }

Copy link
Contributor

@johnsonmh johnsonmh Jun 26, 2020

Choose a reason for hiding this comment

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

And actually, if that did work, then you would be able to condense some of the methods above.

Something like:

  static MaterialStateProperty<T> _lerpProperties(MaterialStateProperty<T> a, MaterialStateProperty<T> b, double t, T Function(T, T, double) lerpFunction ) {
    if (a == null && b == null)
      return null;
    return _LerpProperties<T>(a, b, t, lerpFunction);
  }

And above you would do something like:

 return ButtonStyle(
      textStyle: _lerpProperties<TextStyle>(a?.textStyle, b?.textStyle, t, TextStyle.lerp),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both suggestions are nice boilerplate reducers, I've incorporated them.

/// at the start, and 16 at the end, with an 8 pixel gap in between.
///
/// The [icon] and [label] arguments must not be null.
factory ContainedButton.icon({
Copy link
Contributor

Choose a reason for hiding this comment

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

These two constructors end up being very similar. Do you think it would be reasonable to consider combining ContainedButton and ContainedButton.icon into one constructor? The constructor could have an optional (nullable) icon param, that, when non null, will place itself alongside child?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, the Foo and Foo.icon constructors are how this was factored before. We're not trying for API compatibility here, so there's no need to include the factory again. On the other hand, Foo and Foo.icon have different defaultStyleOf overrides because their default padding is different and because the padding's dependency on textScaleFactor differs. This and the difference in how the child is handled could certainly be blended together into one build method, but separating the two cases with a class (albeit a private one) simplifies that a little.

}

@immutable
class _ContainedButtonDefaultBackground extends MaterialStateProperty<Color> with Diagnosticable {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: double space here and the one below

Suggested change
class _ContainedButtonDefaultBackground extends MaterialStateProperty<Color> with Diagnosticable {
class _ContainedButtonDefaultBackground extends MaterialStateProperty<Color> with Diagnosticable {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@HansMuller HansMuller force-pushed the new_button_universe branch 2 times, most recently from 4052e75 to 0afa020 Compare June 29, 2020 21:26
@AhsanAyaz
Copy link

Pretty excited to see this getting merged :) Was just looking into the ButtonTheme stuff. Glad it is being improved 👍

@AhsanAyaz
Copy link

@HansMuller do let me know if I can help with anything.

@HansMuller
Copy link
Contributor Author

@AhsanAyaz - thanks for the offer! Will keep you in mind as we move forward on the overall theme update project.

@danielgomezrico
Copy link
Contributor

Is there any estimation on when this will be in the stable channel/released?

@HansMuller
Copy link
Contributor Author

It will be part of the next stable release. Along with a migration guide :-).

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@HansMuller HansMuller deleted the new_button_universe branch February 2, 2021 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating the Material Buttons and their Themes