In general, I'm a little skeptical of string based APIs. I can definitely see the value in essentially having a tag/selector like value for determining theme values though. Is there an advantage over using an enumeration - besides visibility?

I agree. Enums have some serious disadvantages which put me off finding a better solution (than Strings):

• They can't be extended. The "type" of subclasses of MaterialButton will be limited to the enum values we define.
• They're difficult for us to extend. Adding an enum value later will break code that switches on the value and doesn't include a default clause.

Some general concerns:

• are we doing too much work here? For example, getTextTheme is called 5 or 6 times in various parts of the buttonTheme helper methods.

• are the tags useful or a bit of a hack?

Consider an alternative design, where instead of having tags we have n different functions which take a MaterialButton and return some RawMaterialButton or other widget. This might be worse in someways, but I think it connects the tag directly to the produced theme value. It also still allows us to avoid computing theme values we don't need while avoiding repetitive calculations.

Might not matter

I don't think we're actually doing too much work, although it's true that some small computations are done repeatedly.

The current design, with its "type" property, is a compromise. It avoids the API explosion that would come from having a complete set of methods for RaisedButton, OutlineButton, FlatButton (and perhaps IconButton and FloatingActionButton in the future).

As long as button is exactly a FlatButton, I think it is faster to check the runtimeType than use an is check. Since the latter has to verify the full type hierarchy.

You could also pass the type as method generic:

double getElevation<T extends materialButton>(T button) {
  if (T == FlatButton) { ... }
}

I guess the above wouldn't work if you wanted to delegate to MaterialButton, and it might not be obvious but inference could mess up users here:

class MyButton extends MaterialButton {

Widget build(_) => RawMaterialButton(foo: theme.getFoo(this)) // <- T is inferred as MyButton instead of MaterialButton
}

If someone wants to write a button that's configured like a FlatButton but builds a little differently, they could subclass FlatButton and override its build method. FlatButtonWithIcon is essentially that.

The generic approach would be a little cheaper, and it would obviate creating classes like FlatButtonWithIcon. It's similar to an earlier approach I'd taken, where a value that represented the button's type was passed along as a MaterialButton property. It makes using the API a bit clunkier, although it also clarifies the meaning of each getFoo(button) method: getElevation(myFlatButton) is a little clearer that we want the FlatButton elevation.

I don't think this would be practical problem with the type parameter approach:

class MyButton extends MaterialButton {

  Widget build(_) => RawMaterialButton(foo: theme.getFoo(this)) // <- T is inferred as MyButton instead of MaterialButton
}

Only because ButtonThemeData doesn't discriminate based on is MaterialButton. The same scenario with a subclass of a type that is part of the rules, say FlatButton, would have to use theme.getFoo<FlatButton>(), to get the "right" answer. I don't know if that qualifies as a footgun or not.

I think the advantage of the is checks is that they will just work. If it's slow we can always revisit

Since this is almost a one-liner I would use ?? instead.

I have an idea to both reduce the number of is checks for the isIcon case and allow users to correctly extend an IconTheme.

Define an empty mixin called MaterialIconButton, or similar.

/// Something something indicates that we use icon theme.s
mixin MaterialIconButton {}

Then make FlatButtonWithIcon and friends mix this in instead of being public class/private constructor.

_FlatButtonWithIcon extends FlatButton with MaterialIconButton { ... }

You can then simplify the isIcon check to a single is:

 bool _isIconButton(MaterialButton button)  => button is MaterialIconButton

And users can mixin this class to make custom subclasses take the icon config as a bonus.

Good idea. I added MaterialButtonWithIconMixin and made the FooButtonWithIcon classes private.

Nit: here and else where it should be a lowercase null-the-value instead of uppcase Null-the-type

OK, done.

For clarity, I would place the "If" first and drop the otherwise.

I tried various permutations of this doc. It needs to convey the fact that the first non-null production is returned (I used "otherwise"). I agree that it's a little clunky as presented, and I wish the derivation of fillColor wasn't so complicated, but I haven't been able to improve it.

This concerns me a bit, we won't break tests that look for OutlineButton will we?

Good point. The tricky Finder wasn't really needed, I've removed it.

I think we should just leave this as part of the regular theme. While it needs to exist for backwards compatibility, ideally we don't want new apps to find it easy to set to anything besides padded.

It's already a property of the button classes and it needs to be funneled through ButtonTheme so that buttons can (just) depend on ButtonThemeData and not on the overall Theme.

I've made the ButtonThemeData.materialTapTargetSize property private. It's initialized by the constructor, which will usually be called by ThemeData, and returned by getMaterialTapTargetSize() as before.

It seemed like a good idea to similarly not expose the new (color) ButtonThemeData color parameters as properties, since they're available via the getFoo(button) methods. I've made them private as well.

I believe you can use the new mixin syntax now: mixin MaterialButtonWithIconMixin { } right @yjbanov ?

The mixin syntax appears to work.

The analyzer and the linter still produce warnings on some valid code, but if you are not triggering those warnings, you should be OK.

Is there a way we could detect a new material theme and switch to the constrained layout behavior?

New themes can specify constrained layout behavior.

Nit: here and next method, extra space

Fixed

Seems like this doc comment got duplicated from above

Oops, copy and pasteo.

You can remove the factory constructor trick, since it isn't possible to instantiate a mixin