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

Update constructor APIs TooltipTheme, ToggleButtonsTheme, PopupMenuTheme #37338

Merged
merged 4 commits into from
Aug 2, 2019

Conversation

HansMuller
Copy link
Contributor

@HansMuller HansMuller commented Jul 31, 2019

The constructor signatures for TooltipTheme, ToggleButtonsTheme, PopupMenuTheme have been incompatibly changed. They require just one theme data parameter.

For example the TooltipTheme constructor was defined like this:

const PopupMenuTheme({
  Key key,
  Color color,
  ShapeBorder shape,
  double elevation,
  TextStyle textStyle,
  Widget child,
}) : data = PopupMenuThemeData(
       color: color,
       shape: shape,
       elevation: elevation,
       textStyle: textStyle,
     ),
     super(key: key, child: child);
}) : assert(data != null), super(key: key, child: child);

final PopupMenuThemeData data;

It's now defined like this:

const PopupMenuTheme({
  Key key,
  @required this.data,
  Widget child,
}) : assert(data != null), super(key: key, child: child);

final PopupMenuThemeData data;

Not having a theme data parameter makes it difficult to override an inherited theme’s configuration. For example to override the inherited PopupMenuTheme one would have to write:

PopupMenuThemeData inheritedTheme = PopupMenuTheme.of(context);
PopupMenuTheme(
  color: Colors.green, // the only property to be changed
  shape: inheritedTheme.shape,
  elevation: inheritedTheme.elevation,
  textStyle: inheritedTheme.textStyle,
  child: myWidgetSubtree,
)

This is tedious, error prone, and likely to rot when TooltipThemeData is extended. It's also inconsistent with Theme and ThemeData. Better to just have a TooltipThemeData valued data constructor parameter, since copyWith can be applied to ToolTipThemeData.

TooltipTheme(
  data: TooltipTheme.of(context).copyWith(
    height: myNewTooltipHeight, // the only property to be changed
  ),
  child: myWidgetSubtree,
)

These APIs were added just last week: PopupMenuTheme #36088 TooltipTheme #36030 ToggleButtonsTheme #34599

Code that had already adopted the new APIs can easily be updated as shown in the example above.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 31, 2019
@HansMuller HansMuller added the c: API break Backwards-incompatible API changes label Jul 31, 2019
Copy link
Contributor

@shihaohong shihaohong left a comment

Choose a reason for hiding this comment

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

LGTM

@HansMuller HansMuller merged commit 873de56 into flutter:master Aug 2, 2019
@HansMuller HansMuller deleted the theme_ctor_update branch August 2, 2019 17:11
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: API break Backwards-incompatible API changes 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.

4 participants