Add useOverlayPortalController #450
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces the new hook Changes
Sequence Diagram(s)sequenceDiagram
participant Widget as Client Widget
participant HookState as _OverlayPortalControllerHookState
participant Controller as OverlayPortalController
Widget->>HookState: Call useOverlayPortalController()
HookState-->>Widget: Return OverlayPortalController instance
Widget->>Controller: Call show()/hide() to toggle visibility
Widget->>HookState: Dispose widget/hook when done
HookState->>Controller: Automatically dispose controller
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
offich
force-pushed
the
feat/use-overlay-portal-controller
branch
from
a990178 to
51e3c56
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/flutter_hooks/lib/src/overlay_portal_controller.dart (1)
3-11: Enhance the hook documentationThe current documentation could be more descriptive. Consider adding:
- A brief description of what an OverlayPortalController is used for
- Example usage
- Description of the optional
keysparameterExample enhancement:
/// Creates a [OverlayPortalController] that will be disposed automatically. +/// +/// The [OverlayPortalController] is used to control the visibility of content in an [OverlayPortal]. +/// This hook ensures proper lifecycle management of the controller. +/// +/// The optional [keys] parameter can be used to preserve the controller's state across rebuilds +/// when the specified keys remain unchanged. +/// +/// Example usage: +/// ```dart +/// final controller = useOverlayPortalController(); +/// return OverlayPortal( +/// controller: controller, +/// overlayChildBuilder: (context) => const Text('Overlay Content'), +/// child: const Text('Anchor Widget'), +/// ); +/// ``` /// /// See also: /// - [OverlayPortalController]packages/flutter_hooks/test/use_overlay_portal_controller_test.dart (1)
34-61: Simplify the test structureThe test could be simplified while maintaining the same verification:
testWidgets('initial values matches with real constructor', (tester) async { late OverlayPortalController controller; final controller2 = OverlayPortalController(); - await tester.pumpWidget(MaterialApp( - home: Scaffold( - body: HookBuilder(builder: (context) { - controller = useOverlayPortalController(); - return Column( - children: [ - OverlayPortal( - controller: controller, - overlayChildBuilder: (context) => - const Text('Overlay Portal'), - ), - OverlayPortal( - controller: controller2, - overlayChildBuilder: (context) => - const Text('Overlay Portal 2'), - ), - ], - ); - }), - ), - )); + await tester.pumpWidget( + HookBuilder(builder: (context) { + controller = useOverlayPortalController(); + return const SizedBox(); + }), + ); expect(controller, isA<OverlayPortalController>()); expect(controller.isShowing, controller2.isShowing); + expect(controller.isShowing, isFalse); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
README.md(1 hunks)packages/flutter_hooks/CHANGELOG.md(1 hunks)packages/flutter_hooks/lib/src/hooks.dart(1 hunks)packages/flutter_hooks/lib/src/overlay_portal_controller.dart(1 hunks)packages/flutter_hooks/test/use_overlay_portal_controller_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/flutter_hooks/lib/src/hooks.dart
- packages/flutter_hooks/CHANGELOG.md
- README.md
🔇 Additional comments (2)
packages/flutter_hooks/lib/src/overlay_portal_controller.dart (1)
13-19: LGTM!
The hook class implementation follows the standard pattern and correctly implements the required methods.
packages/flutter_hooks/test/use_overlay_portal_controller_test.dart (1)
9-31: LGTM!
The test properly verifies the diagnostic representation of the hook.
| class _OverlayPortalControllerHookState | ||
| extends HookState<OverlayPortalController, _OverlayPortalControllerHook> { | ||
| final controller = OverlayPortalController(); | ||
|
|
||
| @override | ||
| OverlayPortalController build(BuildContext context) => controller; | ||
|
|
||
| @override | ||
| String get debugLabel => 'useOverlayPortalController'; | ||
| } |
There was a problem hiding this comment.
Add dispose method to clean up resources
The controller should be disposed when the hook is disposed to prevent memory leaks.
Add the dispose method:
class _OverlayPortalControllerHookState
extends HookState<OverlayPortalController, _OverlayPortalControllerHook> {
final controller = OverlayPortalController();
@override
OverlayPortalController build(BuildContext context) => controller;
@override
String get debugLabel => 'useOverlayPortalController';
+
+ @override
+ void dispose() {
+ controller.dispose();
+ super.dispose();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class _OverlayPortalControllerHookState | |
| extends HookState<OverlayPortalController, _OverlayPortalControllerHook> { | |
| final controller = OverlayPortalController(); | |
| @override | |
| OverlayPortalController build(BuildContext context) => controller; | |
| @override | |
| String get debugLabel => 'useOverlayPortalController'; | |
| } | |
| class _OverlayPortalControllerHookState | |
| extends HookState<OverlayPortalController, _OverlayPortalControllerHook> { | |
| final controller = OverlayPortalController(); | |
| @override | |
| OverlayPortalController build(BuildContext context) => controller; | |
| @override | |
| String get debugLabel => 'useOverlayPortalController'; | |
| @override | |
| void dispose() { | |
| controller.dispose(); | |
| super.dispose(); | |
| } | |
| } |
| testWidgets('check show/hide of overlay portal', (tester) async { | ||
| late OverlayPortalController controller; | ||
| await tester.pumpWidget(MaterialApp( | ||
| home: Scaffold( | ||
| body: HookBuilder(builder: (context) { | ||
| controller = useOverlayPortalController(); | ||
| return OverlayPortal( | ||
| controller: controller, | ||
| overlayChildBuilder: (context) => const Text('Overlay Portal 2'), | ||
| ); | ||
| }), | ||
| ), | ||
| )); | ||
|
|
||
| expect(controller.isShowing, false); | ||
| controller.show(); | ||
| expect(controller.isShowing, true); | ||
| controller.hide(); | ||
| expect(controller.isShowing, false); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for show/hide functionality
The test verifies the controller state but should also verify:
- Widget tree updates after show/hide
- Actual visibility of the overlay content
testWidgets('check show/hide of overlay portal', (tester) async {
late OverlayPortalController controller;
await tester.pumpWidget(MaterialApp(
home: Scaffold(
body: HookBuilder(builder: (context) {
controller = useOverlayPortalController();
return OverlayPortal(
controller: controller,
- overlayChildBuilder: (context) => const Text('Overlay Portal 2'),
+ overlayChildBuilder: (context) => const Text('Overlay Content'),
);
}),
),
));
expect(controller.isShowing, false);
+ expect(find.text('Overlay Content'), findsNothing);
+
controller.show();
+ await tester.pump();
expect(controller.isShowing, true);
+ expect(find.text('Overlay Content'), findsOneWidget);
+
controller.hide();
+ await tester.pump();
expect(controller.isShowing, false);
+ expect(find.text('Overlay Content'), findsNothing);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testWidgets('check show/hide of overlay portal', (tester) async { | |
| late OverlayPortalController controller; | |
| await tester.pumpWidget(MaterialApp( | |
| home: Scaffold( | |
| body: HookBuilder(builder: (context) { | |
| controller = useOverlayPortalController(); | |
| return OverlayPortal( | |
| controller: controller, | |
| overlayChildBuilder: (context) => const Text('Overlay Portal 2'), | |
| ); | |
| }), | |
| ), | |
| )); | |
| expect(controller.isShowing, false); | |
| controller.show(); | |
| expect(controller.isShowing, true); | |
| controller.hide(); | |
| expect(controller.isShowing, false); | |
| }); | |
| testWidgets('check show/hide of overlay portal', (tester) async { | |
| late OverlayPortalController controller; | |
| await tester.pumpWidget(MaterialApp( | |
| home: Scaffold( | |
| body: HookBuilder(builder: (context) { | |
| controller = useOverlayPortalController(); | |
| return OverlayPortal( | |
| controller: controller, | |
| overlayChildBuilder: (context) => const Text('Overlay Content'), | |
| ); | |
| }), | |
| ), | |
| )); | |
| expect(controller.isShowing, false); | |
| expect(find.text('Overlay Content'), findsNothing); | |
| controller.show(); | |
| await tester.pump(); | |
| expect(controller.isShowing, true); | |
| expect(find.text('Overlay Content'), findsOneWidget); | |
| controller.hide(); | |
| await tester.pump(); | |
| expect(controller.isShowing, false); | |
| expect(find.text('Overlay Content'), findsNothing); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/flutter_hooks/lib/src/overlay_portal_controller.dart (1)
3-11: Enhance hook documentation with examples and detailsWhile the basic documentation is present, it would be more helpful to include:
- Usage examples
- Common use cases
- Link to Flutter's
OverlayPortalControllerdocumentationAdd the following to the documentation:
/// Creates a [OverlayPortalController] that will be disposed automatically. /// +/// Example usage: +/// ```dart +/// final controller = useOverlayPortalController(); +/// return OverlayPortal( +/// controller: controller, +/// overlayChildBuilder: (context) => const Text('Overlay Content'), +/// ); +/// ``` +/// /// See also: /// - [OverlayPortalController] +/// - [OverlayPortal]packages/flutter_hooks/test/use_overlay_portal_controller_test.dart (2)
9-31: Enhance debug properties test coverageConsider adding test cases for different controller states (SHOWING, HIDDEN) to ensure complete debug output coverage.
Add the following test cases:
testWidgets('debugFillProperties', (tester) async { await tester.pumpWidget( HookBuilder(builder: (context) { - useOverlayPortalController(); + final controller = useOverlayPortalController(); return const SizedBox(); }), ); await tester.pump(); final element = tester.element(find.byType(HookBuilder)); + + // Test DETACHED state expect( element .toDiagnosticsNode(style: DiagnosticsTreeStyle.offstage) .toStringDeep(), equalsIgnoringHashCodes( 'HookBuilder\n' ' │ useOverlayPortalController: OverlayPortalController DETACHED\n' ' └SizedBox(renderObject: RenderConstrainedBox#00000)\n', ), ); + + // Test SHOWING state + controller.show(); + await tester.pump(); + expect( + element + .toDiagnosticsNode(style: DiagnosticsTreeStyle.offstage) + .toStringDeep(), + equalsIgnoringHashCodes( + 'HookBuilder\n' + ' │ useOverlayPortalController: OverlayPortalController SHOWING\n' + ' └SizedBox(renderObject: RenderConstrainedBox#00000)\n', + ), + ); });
34-61: Simplify and focus the initial values testThe test creates unnecessary complexity by using two controllers. The comparison can be simplified to focus on the essential behavior.
Simplify the test:
testWidgets('initial values matches with real constructor', (tester) async { late OverlayPortalController controller; - final controller2 = OverlayPortalController(); await tester.pumpWidget(MaterialApp( home: Scaffold( body: HookBuilder(builder: (context) { controller = useOverlayPortalController(); - return Column( - children: [ - OverlayPortal( - controller: controller, - overlayChildBuilder: (context) => - const Text('Overlay Portal'), - ), - OverlayPortal( - controller: controller2, - overlayChildBuilder: (context) => - const Text('Overlay Portal 2'), - ), - ], - ); + return OverlayPortal( + controller: controller, + overlayChildBuilder: (context) => const Text('Overlay Portal'), + ); }), ), )); expect(controller, isA<OverlayPortalController>()); - expect(controller.isShowing, controller2.isShowing); + expect(controller.isShowing, false); });packages/flutter_hooks/CHANGELOG.md (1)
1-4: Enhance changelog entry with more detailsWhile the entry follows conventions, it would be helpful to include a brief description of the hook's purpose and use case.
Add more context to the changelog entry:
## Unreleased build -Added `useOverlayPortalController` (thanks to @offich) +Added `useOverlayPortalController` - A hook that creates and manages an `OverlayPortalController` for controlling overlay visibility in Flutter applications (thanks to @offich)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
README.md(1 hunks)packages/flutter_hooks/CHANGELOG.md(1 hunks)packages/flutter_hooks/lib/src/hooks.dart(1 hunks)packages/flutter_hooks/lib/src/overlay_portal_controller.dart(1 hunks)packages/flutter_hooks/test/use_overlay_portal_controller_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/flutter_hooks/lib/src/hooks.dart
🔇 Additional comments (4)
packages/flutter_hooks/lib/src/overlay_portal_controller.dart (2)
13-19: LGTM!
The hook class implementation follows best practices and correctly implements the required methods.
21-30: Add dispose method to clean up resources
The controller should be disposed when the hook is disposed to prevent memory leaks.
packages/flutter_hooks/test/use_overlay_portal_controller_test.dart (1)
63-89: LGTM!
The show/hide test implementation is comprehensive and follows best practices:
- Verifies controller state changes
- Checks widget tree updates
- Validates overlay content visibility
README.md (1)
Line range hint 1-1: Update translations
Since the README has translations in multiple languages (Português, 한국어, 简体中文), consider updating those translation files as well to maintain consistency across all documentation.
Let's check if the translation files exist and need updates:
README.md
Outdated
| @@ -358,6 +358,7 @@ A series of hooks with no particular theme. | |||
| | [useExpansionTileController](https://api.flutter.dev/flutter/material/ExpansionTileController-class.html) | Creates a `ExpansionTileController`. | | |||
| | [useDebounced](https://pub.dev/documentation/flutter_hooks/latest/flutter_hooks/useDebounced.html) | Returns a debounced version of the provided value, triggering widget updates accordingly after a specified timeout duration | | |||
| | [useDraggableScrollableController](https://api.flutter.dev/flutter/widgets/DraggableScrollableController-class.html) | Creates a `DraggableScrollableController`. | | |||
| | [useOverlayPortalController](https://api.flutter.dev/flutter/widgets/OverlayPortalController-class.html) | Creates a `useOverlayPortalController`. | | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the hook description
The current description "Creates a useOverlayPortalController" is circular and doesn't explain the hook's purpose. Consider revising it to better explain its functionality and relationship with Flutter's OverlayPortalController.
-| [useOverlayPortalController](https://api.flutter.dev/flutter/widgets/OverlayPortalController-class.html) | Creates a `useOverlayPortalController`. |
+| [useOverlayPortalController](https://api.flutter.dev/flutter/widgets/OverlayPortalController-class.html) | Creates and manages an `OverlayPortalController` for controlling the visibility of overlay content. The controller will be automatically disposed when no longer needed. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | [useOverlayPortalController](https://api.flutter.dev/flutter/widgets/OverlayPortalController-class.html) | Creates a `useOverlayPortalController`. | | |
| | [useOverlayPortalController](https://api.flutter.dev/flutter/widgets/OverlayPortalController-class.html) | Creates and manages an `OverlayPortalController` for controlling the visibility of overlay content. The controller will be automatically disposed when no longer needed. | |
Fixes #449
Added useOverlayPortalController. I edited readme or changelog like other pull requests. Please let me know if there should be other changes needed.
Summary by CodeRabbit