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

Fix modalDecorator Context Wrapping in WoltModalSheet #340

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

ulusoyca
Copy link
Collaborator

@ulusoyca ulusoyca commented Nov 10, 2024

Description

The modalDecorator in WoltModalSheet was not correctly wrapping the modal content, causing inherited widgets or providers added via modalDecorator to be inaccessible within pageListBuilder. This resulted in exceptions like ProviderNotFoundException when attempting to access context-dependent widgets inside pageListBuilder.

Related Issues

Wolt internal problem.

Solution

The fix involves adjusting the build process in WoltModalSheet to ensure that the modalDecorator is applied before pageListBuilder is called. This way, the context used within pageListBuilder includes the inherited widgets or providers added by modalDecorator.

Changes Made

  1. Modified the build Method in WoltModalSheetState:
  • Checked if a modalDecorator is provided.
  • If so, wrapped the modal content with modalDecorator using a Builder widget to create a new BuildContext that includes the decorations.
  • Moved the original build logic into a new _buildModalContent method, which now uses the decorated context.
  1. Updated Context Usage in _buildModalContent:
  • Ensured that all references to context inside _buildModalContent and any methods it calls use the BuildContext that includes the decorations applied by modalDecorator.
  • Adjusted Initialization of _pages in didChangeDependencies by moving the initialization of _pages into _buildModalContent to ensure it has access to the decorated context.

Example usage after fix

WoltModalSheet.show(
  context: context,
  pageListBuilder: (BuildContext context) {
    final inheritedWidget = MyInheritedWidget.of(context);
    final data = inheritedWidget?.data ?? 'No data found';
    // Use 'data' as needed in your page
    return [RootSheetPage.build(context, data)];
  },
  modalDecorator: (child) {
    return MyInheritedWidget(
      data: 'Hello from InheritedWidget',
      child: child,
    );
  },
  // ... other parameters ...
);

How to test?

  • Clone repository.
  • Run the playground app, and see that it works.
  • Revert the changes in wolt_modal_sheet.dart
  • Run the playground app, and see that it is gives exception when Show Modal Sheet is pressed.
Broken Working

Impact

  • Developers can now use modalDecorator to wrap the entire modal, including the barrier and content, with widgets that provide inherited context.
  • State Management: Facilitates better state management practices by allowing providers or inherited widgets to be accessible throughout the modal's content.
  • Backward Compatibility: This change is backward-compatible and does not affect existing implementations that do not use modalDecorator.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes tests for all changed/updated/fixed behaviors.
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • The package compiles with the minimum Flutter version stated in the pubspec.yaml

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Copy link

It appears that this PR does not include any tests. It is recommended to add tests, especially for critical changes, to ensure code quality and prevent regressions. However, if this PR is only updating samples or documentation, feel free to skip adding tests and disregard this comment.

Copy link
Collaborator

@durannumit durannumit left a comment

Choose a reason for hiding this comment

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

LGTM just added one comment

@@ -332,8 +331,17 @@ class WoltModalSheetState extends State<WoltModalSheet> {
Widget modalContent = ValueListenableBuilder(
valueListenable: widget.pageIndexNotifier,
builder: (context, currentPageIndex, __) {
if (_pages.isEmpty) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can wrap to try-catch for prevent the app from crashing if there’s an issue with pageListBuilderNotifier

Suggested change
if (_pages.isEmpty) {
if (_pages.isEmpty) {
try {
final initialPages = widget.pageListBuilderNotifier.value(context);
assert(
initialPages.isNotEmpty,
'pageListBuilder must return a non-empty list.',
);
_pages = initialPages;
} catch (e) {
debugPrint("Error initializing pages: $e");
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the input. I would like to keep it the current way because it should throw exception if the initial list is empty. We also provide assert which works in debug mode for feedback to devs.

Copy link

It appears that this PR does not include any tests. It is recommended to add tests, especially for critical changes, to ensure code quality and prevent regressions. However, if this PR is only updating samples or documentation, feel free to skip adding tests and disregard this comment.

@ulusoyca ulusoyca enabled auto-merge November 12, 2024 07:55
@ulusoyca ulusoyca disabled auto-merge November 12, 2024 07:58
@ulusoyca ulusoyca merged commit 7c46586 into main Nov 12, 2024
3 of 4 checks passed
@ulusoyca ulusoyca deleted the fix-modal-decorator branch November 12, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants