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: Consider directionality for pagination animation #272

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

ulusoyca
Copy link
Collaborator

Summary

ThisPR introduces support for both Left-to-Right (LTR) and Right-to-Left (RTL) text directions in the slide animation logic for modal sheet page transitions. The mainContentSlidePosition method is updated to handle different slide directions based on a new isLTR parameter.

Changes

Calculated directionMultiplier based on isLTR and isForwardMove to correctly adjust the animation direction.
For LTR (isLTR = true), the multiplier is 1 for forward moves and -1 for backward moves.
For RTL (isLTR = false), the multiplier is -1 for forward moves and 1 for backward moves.

Before LTR After LTR
before_ltr.mov
after_ltr.mov
Before RTL After RTL
before_rtl.mov
after_rtl.mov

Related Issues

Fixes #266

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.
after_rtl.mov

@ulusoyca ulusoyca requested a review from durannumit July 23, 2024 14:33
@ulusoyca ulusoyca changed the title Consider directionality for pagination animation Fix: Consider directionality for pagination animation Jul 23, 2024
Animation<Offset> mainContentSlidePosition(
AnimationController controller,
WoltModalSheetPaginationAnimationStyle style, {
required double sheetWidth,
required double screenWidth,
required bool isForwardMove,
required bool isLTR, // New parameter to determine the text direction
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
required bool isLTR, // New parameter to determine the text direction
required bool isLTR, // New parameter to determine the text direction

Instead of this, we could use a textDirection parameter and pass Directionality.of(context)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this parameter optional? That way we can always default to LTR when this parameter is null.

Copy link
Collaborator Author

@ulusoyca ulusoyca Jul 23, 2024

Choose a reason for hiding this comment

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

Changed the parameter type: d5fab23
I would avoid making it optional. First of all, this is not public API. Second, the maintainers should always consider the directionality when calling this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Second, the maintainers should always consider the directionality when calling this.

I agree, it should be textDirection which is the value of Directionality.of(context) instead a bool parameter for "isLTR'

Animation<Offset> mainContentSlidePosition(
AnimationController controller,
WoltModalSheetPaginationAnimationStyle style, {
required double sheetWidth,
required double screenWidth,
required bool isForwardMove,
required bool isLTR, // New parameter to determine the text direction
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this parameter optional? That way we can always default to LTR when this parameter is null.

@ulusoyca ulusoyca force-pushed the fix-rtl-for-slide-animation branch from 157a0bc to d5fab23 Compare July 23, 2024 15:08
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!

@ulusoyca ulusoyca merged commit 64911e8 into main Jul 23, 2024
3 checks passed
@ulusoyca ulusoyca deleted the fix-rtl-for-slide-animation branch July 23, 2024 15:14
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.

Transition direction for pages ( rtl - ltr )
2 participants