Skip to content

Conversation

@lisa-liao
Copy link
Contributor

@lisa-liao lisa-liao commented Jul 12, 2019

Description

This change introduces a PopupMenuEntryTheme that allows you to theme the color, shape, elevation, and text style of a menu. This can be done at the theme level and at call time of showMenu. See the following image for an example:

Screen Shot 2019-07-12 at 11 54 11 AM

A gist showing how to generate this menu is here.

Related Issues

Closes #36076

Tests

I added the following tests:

*Tests for the theme and modifying the color/shape/elevation/text style of a menu

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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 12, 2019
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

@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.

Good stuff, just a handful of minor comments/questions.

shape: BeveledRectangleBorder(borderRadius: BorderRadius.circular(12)),
elevation: 12.0,
textStyle: const TextStyle(
color: Color(0xffffffff), textBaseline: TextBaseline.alphabetic),
Copy link
Contributor

Choose a reason for hiding this comment

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

Specifying the baseline doesn't seem necessary and can be omitted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

textBaseline is null if left unspecified, and basic.dart requires that textBaseline be non-null.


final AnimatedDefaultTextStyle text =
tester.widget<AnimatedDefaultTextStyle>(
find
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting is a bit weird here, maybe put the find on the same line as descendant here and throughout this file?

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

final Material button = tester.widget<Material>(
find
.descendant(
of: find.byKey(popupButtonApp),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I quite follow how this gets the actual menu widget, could you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last actual piece of material under the popupButtonApp widget is the menu widget itself. So by finding the last descendent of popupButtonApp that is of type Material, this code gets the actual menu widget.

return _cache[key] = loader();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You should undo this change

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

@override
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
super.debugFillProperties(properties);
properties.add(ColorProperty(
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to stick to the 80 char limit so I'd keep everything on the same line so that this is more readable

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

this.textStyle,
});

/// Default value for [_PopupMenuRoute.color].
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an additional line for each property documentation explaining what will be used if this is null. Look at card_theme.dart for examples.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to describe the property itself. I'm unsure if routing developers to look at source code (since it's a private widget) is ideal. See the properties for ToggleButtonsThemeData for examples.

import 'package:flutter/foundation.dart';
import 'package:flutter/rendering.dart';

/// Defines default property values for [PopupMenuEntry]'s [Material].
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not just for material since the text style is in here too, should be enough to just say `[PopupMenuEntry] widgets

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

showMenu<T>(
context: context,
elevation: widget.elevation,
elevation: widget.elevation ?? popupMenuEntryTheme.elevation ?? 8.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this logic (for elevation, shape, and color) repeated in 2 spots, does it have to be?

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 (The paths are essentially combined into 1 now!)

});

/// Default value for [_PopupMenuRoute.color].
final Color surfaceContainerColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a mouthful, color would be enough (and consistent with other themes) I 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.

Done


/// The text style of the entry.
///
/// Defaults to [blackMountainView].
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this doesn't actually default to blackMountainView. You can say something like:

If this is null, then popupMenuTheme.textStyle is used. If that is also null, then theme.textTheme.subhead is used.

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

Copy link
Contributor

@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.

LGTM!

@shihaohong shihaohong self-requested a review July 18, 2019 04:22
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.

Some minor changes to the documentation and removing one line of duplicate logic


/// If provided, the shape used for the menu.
///
/// If this is null, then popupMenuTheme.shape is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If this is null, then popupMenuTheme.shape is used.
/// If this is null, then [PopupMenuThemeData.shape] is used.

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

/// The text style of the entry.
///
/// If this is null, then [popupMenuTheme.textStyle] is used.
/// If that is also null, then [theme.textTheme.subhead] is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If that is also null, then [theme.textTheme.subhead] is used.
/// If that is also null, then [ThemeData.textTheme.subhead] is used.

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


/// The text style of the entry.
///
/// If this is null, then [popupMenuTheme.textStyle] is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If this is null, then [popupMenuTheme.textStyle] is used.
/// If this is null, then [PopupMenuThemeData.textStyle] is used.

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


/// If provided, the color used for the menu.
///
/// If this is null, then popupMenuTheme.color is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If this is null, then popupMenuTheme.color is used.
/// If this is null, then [PopupMenuThemeData.color] is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a default color for the menu if neither are specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If these are null, then Theme.of(context).cardColor is used. If that is also null, then [ThemeData.cardTheme.color] is used. If that is also null, then [ThemeData.cardColor] is used.

/// [PopupMenuEntryThemeData.copyWith].
///
/// Typically a [PopupMenuEntryThemeData] is specified as part of the
/// overall [Theme] with [ThemeData.popupMenuThemeData].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// overall [Theme] with [ThemeData.popupMenuThemeData].
/// overall [Theme] with [ThemeData.popupMenuTheme].

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

return typedOther.elevation == elevation
&& typedOther.color == color
&& typedOther.shape == shape
&& typedOther.elevation == elevation
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line, it's a duplicate check of L99

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

final BottomSheetThemeData bottomSheetTheme;

/// A theme for customizing the color, shape, elevation, and text style of a
/// menu.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// menu.
/// popup menus.

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

/// A theme for customizing the color, elevation, and shape of a bottom sheet.
final BottomSheetThemeData bottomSheetTheme;

/// A theme for customizing the color, shape, elevation, and text style of a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A theme for customizing the color, shape, elevation, and text style of a
/// A theme for customizing the color, shape, elevation, and text style of

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

@lisa-liao lisa-liao changed the title Add PopupMenuEntryTheme to enable theming color, shape, elevation, text style of Menu Add PopupMenuTheme to enable theming color, shape, elevation, text style of Menu Jul 18, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

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.

There's a small bug where the widgets do not check for the PopupMenuTheme inherited widget, it would probably be good to add a test for that to validate the right behavior. I also added some comments about docs and formatting.

@override
final double height;

/// The text style of the entry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The text style of the entry.
/// The text style of the popup menu entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

We try to make the first sentence as clear as possible so developers can very quickly understand what the API is for, or be reminded if they forget.

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

Widget build(BuildContext context) {
final ThemeData theme = Theme.of(context);
TextStyle style = theme.textTheme.subhead;
final PopupMenuThemeData popupMenuTheme = theme.popupMenuTheme;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final PopupMenuThemeData popupMenuTheme = theme.popupMenuTheme;
final PopupMenuThemeData popupMenuTheme = TooltipTheme.of(context);

Copy link
Contributor

Choose a reason for hiding this comment

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

We want to retrieve the PopupMenuTheme configuration data from the closest ancestor in the widget tree.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe also add a test to verify that when

  1. ThemeData.popupTheme is defined, those properties are utilized.
  2. When ThemeData.popupTheme is defined, but a PopupTheme is also defined as a closer ancestor to any popup menu, properties from PopupThemeData are used instead.

Widget build(BuildContext context) {
final double unit = 1.0 / (route.items.length + 1.5); // 1.0 for the width and 0.5 for the last item's fade.
final List<Widget> children = <Widget>[];
final PopupMenuThemeData popupMenuTheme = Theme.of(context).popupMenuTheme;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final PopupMenuThemeData popupMenuTheme = Theme.of(context).popupMenuTheme;
final PopupMenuThemeData popupMenuTheme = TooltipTheme.of(context);

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto over here

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


/// The text style of the entry.
///
/// If this is null, then [PopupMenuThemeData.textStyle] is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If this is null, then [PopupMenuThemeData.textStyle] is used.
/// If this property is null, then [PopupMenuThemeData.textStyle] is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using 'this' or 'that', since it can be ambiguous

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

/// The text style of the entry.
///
/// If this is null, then [PopupMenuThemeData.textStyle] is used.
/// If that is also null, then [ThemeData.textTheme.subhead] is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If that is also null, then [ThemeData.textTheme.subhead] is used.
/// If [PopupMenuThemeData.textStyle] is also null, then [ThemeData.textTheme.subhead] is used.

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

expect(button.shape, null);
expect(button.elevation, 8.0);

final AnimatedDefaultTextStyle text =
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about formatting

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

expect(button.shape, popupMenuTheme.shape);
expect(button.elevation, popupMenuTheme.elevation);

final AnimatedDefaultTextStyle text =
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting here

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

expect(button.shape, shape);
expect(button.elevation, elevation);

final AnimatedDefaultTextStyle text =
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting here

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

// found in the LICENSE file.

import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the analyzer might be complaining about this line being a recursive import. Just import the particular files you need and not the entire material library

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

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

CupertinoThemeData cupertinoOverrideTheme,
SnackBarThemeData snackBarTheme,
BottomSheetThemeData bottomSheetTheme,
PopupMenuTheme popupMenuTheme,
Copy link
Contributor

@rami-a rami-a Jul 23, 2019

Choose a reason for hiding this comment

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

This should still be PopupMenuThemeData for the type

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

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.

I have some comments about formatting and the API documentation. Also, the analyzer seems to be complaining about something

this.semanticLabel,
this.shape,
this.color,
this.popupMenuTheme,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this up under theme, so that the list of parameters is more intuitively ordered

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

}

Widget menu = _PopupMenu<T>(route: this, semanticLabel: semanticLabel);
if(popupMenuTheme != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(popupMenuTheme != null)
if (popupMenuTheme != null)

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

barrierLabel: MaterialLocalizations.of(context).modalBarrierDismissLabel,
shape: shape,
color: color,
popupMenuTheme: PopupMenuTheme.of(context),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as prior comment, make this right under theme. In general, we try to order parameters if there is a logical way to do so

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


/// If provided, the shape used for the menu.
///
/// If this property is null, then [PopupMenuThemeData.shape] is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Describe what the shape would be if PopupMenuThemeData.shape was null as well.

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


/// If provided, the color used for the menu.
///
/// If this property is null, then [PopupMenuThemeData.color] is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Describe what the color would be if PopupMenuThemeData.color was null as well.

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

).last,
);
expect(button.color, Colors.pink);
expect(button.shape,
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this one line, or fix formatting

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

});
}

PopupMenuThemeData _popupMenuTheme() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to put this at the top of the file. I believe most test files in the framework follow that convention

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 (I was looking at the Bottom Sheet Theme which seems to have this at the bottom of the test file?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it must be a preference thing. The files I've worked on had them at the top of the file. Sorry about that!

home: Material(
child: Column(
children: <Widget>[
PopupMenuButton<void>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove <void>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The analyzer says that we can't remove <void> because type annotations must be specified.

home: Material(
child: Column(
children: <Widget>[
PopupMenuButton<void>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove <void>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The analyzer says that we can't remove <void> because type annotations must be specified.

shape: BeveledRectangleBorder(borderRadius: BorderRadius.circular(10)),
elevation: 6.0,
textStyle: const TextStyle(color: Color(0xfffff000), textBaseline: TextBaseline.alphabetic),
child: PopupMenuButton<void>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove <void>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The analyzer says that we can't remove <void> because type annotations must be specified.

@shihaohong shihaohong self-requested a review July 23, 2019 23:13
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.

One tiny nit about the API doc, otherwise LGTM!

You might also have to merge your changes with master. Some of the presubmit tests are complaining that seem unrelated to your changes.

///
/// If this property is null, then [PopupMenuThemeData.color] is used.
/// If [PopupMenuThemeData.color] is also null, then
/// [Theme.of(context).cardColor] is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, since this wouldn't create the link you're expecting:

Suggested change
/// [Theme.of(context).cardColor] is used.
/// Theme.of(context).cardColor is used.

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

@shihaohong shihaohong self-assigned this Jul 24, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@shihaohong
Copy link
Contributor

I'm manually updating the CLA since the PR contributor is a Googler


/// Defines the visual properties of [PopupMenuEntry] widgets.
///
/// Descendant widgets obtain the current [PopupMenuEntryThemeData] object
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like PopupMenuEntry* should be PopupMenu* in the next couple of paragraphs.

Also: apps and material widgets should use PopupMenuTheme.of(context) (which defaults Theme.of(context).popupMenuTheme)).

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:
///
/// * [ThemeData], which describes the overall theme information for the
/// application.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these links are needed.


import 'theme.dart';

/// Defines the visual properties of [PopupMenuEntry] widgets.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be clearer to say ...properties of the routes used to display popup menus as well as [PopupMenuItem] and [PopupMenuDivider] widgets.

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

/// The properties for descendant popup menu widgets.
final PopupMenuThemeData data;

/// The closest instance of this class' [data] value that encloses the given
Copy link
Contributor

Choose a reason for hiding this comment

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

class' should be class's. English is strange.

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

expect(button.shape, popupMenuTheme.shape);
expect(button.elevation, popupMenuTheme.elevation);

final AnimatedDefaultTextStyle text = tester.widget<AnimatedDefaultTextStyle>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just use DefaultTextStyle here

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

find.descendant(
of: find.byKey(popupButtonApp),
matching: find.byType(Material),
).last,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear what the ".last" material widget is. Maybe add a comment (here and below).

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

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

Nice debut!

@rami-a rami-a added cla: yes and removed cla: no labels Jul 26, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@rami-a
Copy link
Contributor

rami-a commented Jul 26, 2019

Overriding CLA as @lisa-liao is a googler

@rami-a rami-a merged commit 63992e4 into flutter:master Jul 26, 2019
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
…yle of Menu (flutter#36088)

* [Menu] Create Menu theme

* [Menu] Create Menu theme

* [Menu] Formatting changes for Menu theme

* [Menu] Fix spacing difference in theme_data.dart.

* [Menu] Fix spacing difference in theme_data.dart.

* Specifying types

* Formatting changes

* Address PR feedback

* Formatting changes

* Address PR feedback

* Add inherited widget

* Add inherited widget

* Address PR feedback and add inherited widget.

* Formatting changes.

* Address PR feedback

* Address PR feedback

* Address PR feedback

* Address PR feedback
@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

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.

Implement color, shape, elevation, and text style support for menus

6 participants