PopupMenuButton menus inherit the button theme#6132
PopupMenuButton menus inherit the button theme#6132HansMuller merged 2 commits intoflutter:masterfrom
Conversation
| this.initialValue, | ||
| this.elevation | ||
| this.elevation, | ||
| this.theme |
There was a problem hiding this comment.
when you add code like this, add trailing commas :-)
| Theme({ | ||
| Key key, | ||
| @required this.data, | ||
| this.isShadowTheme: true, |
There was a problem hiding this comment.
boolean arguments should when possible default to false (because foo: false looks like something that could be safely removed without effect). Maybe isPrimaryTheme with the reverse semantics? Or isMainTheme? isMaterialAppTheme?
There was a problem hiding this comment.
I went with isMaterialAppTheme since that's part and parcel of the definition.
| /// Specifies the color and typography values for descendant widgets. | ||
| final ThemeData data; | ||
|
|
||
| /// True if this theme shadows the one created by the [MaterialApp]. |
There was a problem hiding this comment.
you should add some color here saying why you would set this (or rather, that you wouldn't), how it's used, etc.
| /// If [shadowTheme] is true and the closest Theme ancestor was installed by | ||
| /// the [MaterialApp] - in other words if the closest Theme ancestor does not | ||
| /// shadow the app's theme - then return null. | ||
| static ThemeData of(BuildContext context, { bool shadowTheme: false }) { |
There was a problem hiding this comment.
maybe shadowThemeOnly? Or secondaryThemeOnly if you call the other argument isPrimaryTheme?
There was a problem hiding this comment.
I used shadowThemeOnly. Thankfully this parameter will rarely be used.
| /// | ||
| /// If [shadowTheme] is true and the closest Theme ancestor was installed by | ||
| /// the [MaterialApp] - in other words if the closest Theme ancestor does not | ||
| /// shadow the app's theme - then return null. |
There was a problem hiding this comment.
add another paragraph talking about why you would use this argument.
|
As discussed, I suspect the other ones need it too because they probably have Material widgets and so forth. LGTM. |
| ThemeData theme = config.theme ?? new ThemeData.fallback(); | ||
| Widget result = new AnimatedTheme( | ||
| data: theme, | ||
| isShadowTheme: false, |
There was a problem hiding this comment.
Maybe isShadowingTheme would be more explicit?
4 participants
If the theme inherited by a PopupMenuButton shadows the material app's theme then the menu itself will be wrapped with the shadow theme. In the common case, where only the app creates a theme widget, the menu inherits the app's theme as usual.
Likewise for dialogs, modal bottom sheets, and drop down menus.
Fixes #5572