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

[Breaking Change] Introduce modal and page content decorators #267

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

ulusoyca
Copy link
Collaborator

Summary

This PR refactors the modal decoration mechanism to separate the decoration of the modal barrier + content from the decoration of the content only. This also aligns it with the WoltModalType class methods, and resolves the existing issue with the decorator context (see comment).

Methods in WoltModalType class: decoratePageContent and decorateModal:

  /// Applies additional decorations to the modal page content.
  ///
  /// This method can be overridden to provide custom decorations such as safe area padding
  /// adjustments inside the modal page content. By default, it does not apply any decoration.
  /// See the [WoltBottomSheetType] for an example of how overriding this method could be used to
  /// fill the bottom safe area.
  Widget decoratePageContent(
    BuildContext context,
    Widget child,
    bool useSafeArea,
  ) =>
      child;

  /// Applies additional decorations to the modal content.
  ///
  /// This method can be overridden to provide custom decorations such as safe area padding
  /// adjustments around the modal including the barrier. By default, it applies safe area
  /// constraints if [useSafeArea] is `true`.
  Widget decorateModal(
    BuildContext context,
    Widget modal,
    bool useSafeArea,
  ) =>
      useSafeArea ? SafeArea(child: modal) : modal;

This PR introduces pageContentDecorator and modalDecorator:

  /// Applies additional decorations to the modal page content excluding the
  /// barrier. Use [modalDecorator] to apply decorations to the barrier and
  /// the content.
  Widget Function(Widget)? pageContentDecorator;

  /// Applies additional decorations to the modal including the barrier and the content.
  Widget Function(Widget)? modalDecorator;

Changes Made to Decorator Functions

Removed:

final Widget Function(Widget)? decorator;
Widget Function(Widget) get _decorator => widget.decorator ?? (widget) => Builder(builder: (_) => widget);

Added:

final Widget Function(Widget)? pageContentDecorator;
final Widget Function(Widget)? modalDecorator;
Widget Function(Widget) get _pageContentDecorator => widget.pageContentDecorator ?? (widget) => Builder(builder: (_) => widget);
Widget Function(Widget) get _modalDecorator => widget.modalDecorator ?? (widget) => Builder(builder: (_) => widget);

Benefits

The decorator function is split into pageContentDecorator and modalDecorator. This fixes the issue stated by @vishna in this comment. Previously, the decorator function only decorated the page modal content, leading to errors when the pageListBuilder created the pages because the decorator context was built after the pages are built.

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

@ulusoyca ulusoyca force-pushed the introduce-modal-decorator-and-modal-content-decorator branch from b282975 to 7d564cd Compare July 19, 2024 12:44
Copy link
Collaborator

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 278 to 283
Widget Function(Widget) get _pageContentDecorator =>
widget.pageContentDecorator ??
(widget) => Builder(builder: (_) => widget);

Widget Function(Widget) get _modalDecorator =>
widget.modalDecorator ?? (widget) => Builder(builder: (_) => widget);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could get rid of the default builders and only apply when decorator builders are not null.

To save you time, i will push the commit with the suggestion. Please feel free to revert the commit if not agreed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is the applied commit 1406281

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TahaTesser <3 you are amazing!

@ulusoyca ulusoyca merged commit fccd27c into main Jul 22, 2024
2 of 3 checks passed
@ulusoyca ulusoyca deleted the introduce-modal-decorator-and-modal-content-decorator branch July 22, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants