Skip to content
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

Add useOverlayPortalController #450

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`. |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
| [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. |


## Contributions

Expand Down
4 changes: 4 additions & 0 deletions packages/flutter_hooks/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## Unreleased build

- Added `useOverlayPortalController` (thanks to @offich)

## 0.21.1-pre.4 - 2024-08-01

- Added `useFixedExtentScrollController` (thanks to @whynotmake-it)
Expand Down
1 change: 1 addition & 0 deletions packages/flutter_hooks/lib/src/hooks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ part 'keep_alive.dart';
part 'listenable.dart';
part 'listenable_selector.dart';
part 'misc.dart';
part 'overlay_portal_controller.dart';
part 'page_controller.dart';
part 'platform_brightness.dart';
part 'primitives.dart';
Expand Down
30 changes: 30 additions & 0 deletions packages/flutter_hooks/lib/src/overlay_portal_controller.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
part of 'hooks.dart';

/// Creates a [OverlayPortalController] that will be disposed automatically.
///
/// See also:
/// - [OverlayPortalController]
OverlayPortalController useOverlayPortalController({
List<Object?>? keys,
}) {
return use(_OverlayPortalControllerHook(keys: keys));
}

class _OverlayPortalControllerHook extends Hook<OverlayPortalController> {
const _OverlayPortalControllerHook({List<Object?>? keys}) : super(keys: keys);

@override
HookState<OverlayPortalController, Hook<OverlayPortalController>>
createState() => _OverlayPortalControllerHookState();
}

class _OverlayPortalControllerHookState
extends HookState<OverlayPortalController, _OverlayPortalControllerHook> {
final controller = OverlayPortalController();

@override
OverlayPortalController build(BuildContext context) => controller;

@override
String get debugLabel => 'useOverlayPortalController';
}
Comment on lines +21 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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();
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter_hooks/src/framework.dart';
import 'package:flutter_hooks/src/hooks.dart';

import 'mock.dart';

void main() {
testWidgets('debugFillProperties', (tester) async {
await tester.pumpWidget(
HookBuilder(builder: (context) {
useOverlayPortalController();
return const SizedBox();
}),
);

await tester.pump();

final element = tester.element(find.byType(HookBuilder));

expect(
element
.toDiagnosticsNode(style: DiagnosticsTreeStyle.offstage)
.toStringDeep(),
equalsIgnoringHashCodes(
'HookBuilder\n'
' │ useOverlayPortalController: OverlayPortalController DETACHED\n'
' └SizedBox(renderObject: RenderConstrainedBox#00000)\n',
),
);
});

group('useOverlayPortalController', () {
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'),
),
],
);
}),
),
));
expect(controller, isA<OverlayPortalController>());
expect(controller.isShowing, controller2.isShowing);
});

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);
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage for show/hide functionality

The test verifies the controller state but should also verify:

  1. Widget tree updates after show/hide
  2. 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.

Suggested change
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);
});

});
}