-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Make sure that a MenuItemButton doesn't crash in 0x0 environment #176419
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
Make sure that a MenuItemButton doesn't crash in 0x0 environment #176419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a regression test to ensure that MenuItemButton does not crash when rendered in a zero-sized area. The new test case wraps a MenuItemButton within a SizedBox.shrink() to simulate a zero-area environment and asserts that the widget's size is indeed zero, ensuring that no crash occurs under these conditions. The test is well-written and accurately covers the reported issue. I have one minor suggestion to improve code style.
| testWidgets('MenuItemButton does not crash at zero area', (WidgetTester tester) async { | ||
| await tester.pumpWidget( | ||
| const MaterialApp( | ||
| home: Center(child: SizedBox.shrink(child: MenuItemButton())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better performance and adherence to Dart best practices, it's recommended to use const for widgets that can be constant. The Center widget and its children can be declared as const here.1
| home: Center(child: SizedBox.shrink(child: MenuItemButton())), | |
| home: const Center(child: SizedBox.shrink(child: MenuItemButton())), |
Style Guide References
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MaterialApp is already const.
| testWidgets('MenuItemButton does not crash at zero area', (WidgetTester tester) async { | ||
| await tester.pumpWidget( | ||
| const MaterialApp( | ||
| home: Center(child: SizedBox.shrink(child: MenuItemButton())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MaterialApp is already const.
dkwingsmt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checklist:
- The test is in the correct file
- The test name goes “does not crash at zero area”
- The target widget is wrapped by
Center(or is fullscreen) - The target widget does not have an overlay, or the overlay is tested
- The target widget is expected to have a size of exactly
Size.zero
|
autosubmit label was removed for flutter/flutter/176419, because - The status or check suite Linux_android_emu native_assets_android has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/176419, because - The status or check suite Linux_android_emu native_assets_android has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/176419, because Pull request flutter/flutter/176419 is not in a mergeable state. |
…tter#176419) This is my attempt to handle flutter#6537 for the MenuItemButton widget. Co-authored-by: Tong Mu <[email protected]>
This is my attempt to handle #6537 for the MenuItemButton widget.