-
Notifications
You must be signed in to change notification settings - Fork 512
fix: clicking on CupertinoContextMenuAction
doesn't close context menu
#3948
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
Conversation
Reviewer's Guide by SourceryThis pull request addresses an issue with the CupertinoContextMenuAction not closing the context menu when clicked. It also includes minor improvements to error messages, type annotations, and removes debug printing. File-Level Changes
Tips
|
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.
Hey @ndonkoHenri - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining the reason for calling Navigator.of(context).pop() in the CupertinoContextMenuAction's onPressed callback.
- Verify that removing the DefaultTextStyle wrapper in CupertinoContextMenuAction doesn't negatively impact text styling. If it's intentional, consider adding a comment explaining the change.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
* main: (31 commits) Migrate `colors` and `icons` variables to Enums (flet-dev#4180) feat: expose more properties in Controls (flet-dev#4105) feat!: Refactor `Badge` Control to a Dataclass; create new `Control.badge` property (flet-dev#4077) fix: clicking on `CupertinoContextMenuAction` doesn't close context menu (flet-dev#3948) fix dropdown `max_menu_height` (flet-dev#3974) Fix undefined name "Future" --> asyncio.Future (flet-dev#4230) wrap ListTile in material widget (flet-dev#4206) Update CONTRIBUTING.md (flet-dev#4208) TextField: suffix_icon, prefix_icon and icon can be Control or str (flet-dev#4173) feat!: enhance `Map` control (flet-dev#3994) skip running flutter doctor on windows if no_rich_output is True (flet-dev#4108) add --pyinstaller-build-args to pack cli command (flet-dev#4187) fix/feat: make `SearchBar`'s view height adjustable; add new properties (flet-dev#4039) fix: prevent button `style` from being modified in `before_update()` (flet-dev#4181) fix: disabling filled buttons is not visually respected (flet-dev#4090) when `label` is set, use `MainAxisSize.min` for the `Row` (flet-dev#3998) fix: `NavigationBarDestination.disabled` has no visual effect (flet-dev#4073) fix autofill in CupertinoTextField (flet-dev#4103) Linechart: jsonDecode tooltip before displaying (flet-dev#4069) fixed bgcolor, color and elevation (flet-dev#4126) ...
Summary by Sourcery
Fix the issue where clicking on a
CupertinoContextMenuAction
does not close the context menu by adding a pop action. Improve the error message inCupertinoContextMenu
for better clarity and remove an unnecessary debug print statement fromPopupMenuButtonControl
. Correct the return type ofget_input_devices_async
inaudio_recorder.py
to return a dictionary instead of a boolean.Bug Fixes:
CupertinoContextMenuAction
closes the context menu by adding a call toNavigator.of(context).pop()
in the onPressed callback.Enhancements:
CupertinoContextMenu
to improve clarity by fixing a grammatical error.Chores:
PopupMenuButtonControl
to clean up the code.