Make the CupertinoContextMenuAction trailing icon themable #45035
Conversation
fluttergithubbot
added
f: cupertino
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@KagurazakaHanabi Thanks for the PR and so sorry for taking so long to review! I agree with this idea, but this would be a breaking change, which Flutter tries to avoid as much as possible. I'm going to think about how we want to enable this kind of customization... By the way, if we do move forward with this PR, we'll need some tests. |
|
Also heads up that the isDestructiveAction parameter wasn't working, but after this PR it will be able to be used to turn the action red: #47151 See also isDefaultAction. |
|
But I think if it is |
|
Sounds good. Yeah I agree that it's more flexible as a widget. |
…bi-patch-2 # Conflicts: # packages/flutter/lib/src/cupertino/context_menu_action.dart
|
Hey, I just saw this, and I think there is a good way to add this customization. Just like Container.color in fact sets an BoxDecoration with this color, trailingIcon can set trailing to be an Icon(trailingIcon) and asserted that either it or trailing is null. This way it won't be a breaking change as trailingIcon will still be available and it can be marked as deprecated, and instructions to migrate can be added to the trailing member documentation. |
|
That's not a bad idea. So there would be both |
|
@justinmc My idea is delete "trailingIcon" directly without considering compatibility, because moving from "trailingIcon" to "trailing" is easy |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Hi @KagurazakaHanabi thank you for the contribution, and apologies for the delayed review! As this is right now, it would be a very large breaking change. I've linked the guide for handling those below.
| return _kActionSheetActionStyle; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: Unnecessary white space
| await tester.pumpWidget(_getApp(isDefaultAction: true)); | ||
| expect(_getTextStyle(tester).fontWeight, _kDefaultActionWeight); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Nit: Unnecessary white space
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Hi @KagurazakaHanabi thanks for checking out the breaking change policy! It also specifies that a design document will need to be created first to discuss the proposed change and receive feedback. The policy has a design doc template that you can use. :)
|
@Piinks I wrote a design doc and migration guide: https://flutter.dev/go/cupertino-context-menu-action |
|
The design doc looks good to me. @KagurazakaHanabi can you continue with the steps in the breaking change policy (here)? |
|
Hi @KagurazakaHanabi are you planning on continuing with this change? |
7 participants
Description
Before: we can't change the trailing icon color and size
Related Issues
Closes #44567
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.