Rewrite CupertinoActionSheet #149334
Rewrite CupertinoActionSheet #149334
Conversation
github-actions
bot
added
framework
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
overall I am on board with this refactor, my comments are mostly in the implementation details
| super.dispose(); | ||
| } | ||
|
|
||
| bool hasContent() => widget.title != null || widget.message != null; |
| } | ||
|
|
||
| /// A button typically used in a [CupertinoActionSheet]. | ||
| /// The content of a typical button in a [CupertinoActionSheet]. |
There was a problem hiding this comment.
| /// The content of a typical button in a [CupertinoActionSheet]. | |
| /// The content of a typical action button in a [CupertinoActionSheet]. |
| final Widget? child; | ||
| final bool isCancel; | ||
|
|
||
| /// The callback that is called when the button is tapped. |
There was a problem hiding this comment.
This comment and variable name don't seem to match
There was a problem hiding this comment.
I've changed it to:
/// Called when the user taps down or lifts up on the button.
///
/// The boolean value is true if the user is tapping down on the button.| const _CupertinoActionSheetCancelButton({ | ||
| this.child, | ||
| // This widget renders the background (pressed or not) of a button, and sends | ||
| // whether the button is pressed to the parent. |
There was a problem hiding this comment.
a bit confused about this doc string, will this even send when the button is not pressed?
| const _CupertinoActionSheetCancelButton({ | ||
| this.child, | ||
| // This widget renders the background (pressed or not) of a button, and sends | ||
| // whether the button is pressed to the parent. |
There was a problem hiding this comment.
maybe
// This widget renders the background (pressed or not) of a button and calls `onPressStateChange`
// when user tap down or lift up on the button
There was a problem hiding this comment.
Thanks, I've changed it to
// Renders the background of a button (both the pressed background and the idle
// background) and reports its state to the parent with `onPressStateChange`.(The exact definition of "the state" is explained in onPressStateChange.) What do you think of it?
| // The buttons and the dividers are placed alternately, both keyed by indexes. | ||
| // Dividers use this special class so that the two lists of indexes can be told | ||
| // apart. | ||
| class _DividerKey extends ValueKey<int> { |
There was a problem hiding this comment.
I am curious why this is needed, are you seeing some state lost due if not using this?
There was a problem hiding this comment.
I wanted to add keys to each element of the list. Since there are two list of integers, I can't add, for example, ValueKey<int>(1) to both the second item and the first divider (I've tested that in this case Flutter will report duplicate key error.)
If you mean that we don't need keys, well, I'm not strong on it either. But I was thinking that adding keys to a list of items might improve the performance, or at least is a good practice. What do you think?
There was a problem hiding this comment.
key in a multichildrenderobject will only matter if you reorder or add or remove child in between update and you want to maintain state, otherwise it is the same as unkeyed, so I will probably not bother adding one for the divider
| await tester.pump(); | ||
|
|
||
| expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(112.3)); | ||
| expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(84.0)); |
There was a problem hiding this comment.
same for other in the file
There was a problem hiding this comment.
On a second thought, I think I should revert this change and propose it in a separate PR.
There was a problem hiding this comment.
I've reverted these changes (and changed the minimum height from 84.0 to 84.3). Now all previous tests pass without any adjustment!
|
I've addresses all comments and made changes so that all existing tests pass without adjustment. |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
dkwingsmt
added
the
autosubmit
dkwingsmt
changed the title
2 participants
This PR rewrite
CupertinoActionSheetto be based on layout widgets instead of the custom layout code in_CupertinoDialogRenderWidget, making it much easier to understand. It also fixes a layout deviation from SwiftUI, and add a few tests._CupertinoDialogRenderWidgetis a custom layout widget that contains too much boilerplate code and is hard to understand where is customized. The plan is to replace it with widgets as much as possible. (Since this widget is used by bothCupertinoActionSheetandCupertinoAlertDialog, this PR only removes the branches related toisActionSheet).The need to use custom layout widget probably came from the following difficulties:
Column.Luckily, I've found out that all these problems are solvable with widgets.
All existing tests pass.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.