Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. by nploi · Pull Request #154048 · flutter/flutter

nploi · 2024-08-24T10:09:24Z

Currently we don't support custom transition duration for DialogRoute, CupertinoDialogRoute and show dialog methods , This PR will to support that.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2024-08-24T10:09:27Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

Piinks

Hey @nploi thanks for the contribution! Can you add a test that verifies it works as expected? That will also ensure we do not undo this work in the future. Thanks!

…te unit-test

nploi · 2024-08-31T05:14:32Z

Hey @nploi thanks for the contribution! Can you add a test that verifies it works as expected? That will also ensure we do not undo this work in the future. Thanks!

Hi @Piinks, i added the tests for my changes. pls help to review. Thank you.

nate-thegrate

I left a couple of suggestions, but overall this is a fantastic improvement. Thanks!

nate-thegrate

LGTM, thanks for making this improvement!

piedcipher

some smol clean-up

nploi · 2024-09-05T15:44:37Z

some smol clean-up

thank for your review, i just fixed, pls help me check again 👍

dkwingsmt

The changes look OK. The new argument needs documentation though.

dkwingsmt · 2024-09-05T15:53:04Z

  bool barrierDismissible = false,
  RouteSettings? routeSettings,
  Offset? anchorPoint,
+  Duration? transitionDuration = _kCupertinoDialogRouteTransitionDuration,


Add a paragraph to the doc to explain the transitionDuration and its default value, such as

/// /// The `transitionDuration` argument determines the duration of the transition animation. If it's /// not provided or null, then it uses the default value as set by [CupertinoDialogRoute].

And also, either use the default value, or use a nullable value, but not default and nullable at the same time. I suggest using a nullable value so that the default duration is determined by CupertinoDialogRoute alone, meaning changing this line to

Suggested change

Duration? transitionDuration = _kCupertinoDialogRouteTransitionDuration,

Duration? transitionDuration,

and removing the ?? in L1330.

dkwingsmt · 2024-09-05T15:54:02Z

    String? barrierLabel,
    // This transition duration was eyeballed comparing with iOS
-    super.transitionDuration = const Duration(milliseconds: 250),
+    super.transitionDuration = _kCupertinoDialogRouteTransitionDuration,


Add a paragraph to the doc of the constructor to explain the transitionDuration and its default value.

dkwingsmt · 2024-09-05T15:58:22Z

    themes: themes,
    anchorPoint: anchorPoint,
    traversalEdgeBehavior: traversalEdgeBehavior ?? TraversalEdgeBehavior.closedLoop,
+    transitionDuration: transitionDuration ?? _kDialogRouteTransitionDuration,


As suggested above,

Suggested change

transitionDuration: transitionDuration ?? _kDialogRouteTransitionDuration,

transitionDuration: transitionDuration,

and add to doc.

dkwingsmt · 2024-09-05T15:58:37Z

  RouteSettings? routeSettings,
  Offset? anchorPoint,
  TraversalEdgeBehavior? traversalEdgeBehavior,
+  Duration? transitionDuration,


As suggested above, add to doc.

dkwingsmt · 2024-09-05T15:58:51Z

    super.requestFocus,
    super.anchorPoint,
    super.traversalEdgeBehavior,
+    super.transitionDuration = _kDialogRouteTransitionDuration,


Add to doc and explain default value.

piedcipher

LGTM

can we add set of tests for testing defaults too?

nploi · 2024-09-06T02:26:19Z

Looks like Linux Analyze is reporting a few trailing whitespace characters.

fixed.

nate-thegrate · 2024-09-06T18:42:20Z

@nploi thanks for addressing all the feedback!

…inoDialogRoute` and show dialog methods. (flutter/flutter#154048)

TahaTesser · 2024-09-06T19:02:28Z

@dkwingsmt @nate-thegrate
We introduced AnimationStyle class to override animation durations and curves.

Instead of introducing new durations and curves parameters, we used this class to override transitionDuration
For instance here

flutter/packages/flutter/lib/src/material/popup_menu.dart

Line 909 in 2e221e7

    
           Duration get transitionDuration => popUpAnimationStyle?.duration ?? _kMenuDuration;

TahaTesser · 2024-09-06T19:06:38Z

Just below this newly added transitionDuration, there are hard coded animation curves. If and when there is request to add support to customize these curves, we wouldn't want to add more new parameters.

Instead we could use AnimationStyle to override durations as well as curve, something like TextStyle for text

TahaTesser · 2024-09-06T19:08:00Z

I would recommend reverting this PR in favor of AnimationStyle for complete animation duration and curve customization.

nate-thegrate · 2024-09-06T19:10:56Z

Reason for revert: using AnimationStyle instead would allow for complete animation duration and curve customization.

…inoDialogRoute` and show dialog methods. (#154048)" This reverts commit 0eaeb0d.

…tinoDialogRoute` and show dialog methods. (#154048)" (#154743) Reverts: #154048 Initiated by: nate-thegrate Reason for reverting: using `AnimationStyle` instead would allow for complete animation duration and curve customization. Original PR Author: nploi Reviewed By: {piedcipher, nate-thegrate} This change reverts the following previous change: Currently we don't support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods , This PR will to support that.

TahaTesser · 2024-09-06T19:17:38Z

Since AnimationStyle is relatively new, for every widget I included a sample. We should also add sample for this change too.
Please look at an existing animation style sample for default, custom, and no animation controls. You can copy this and replace the widget using new parameter.
https://api.flutter.dev/flutter/material/PopupMenuButton-class.html#material.PopupMenuButton.4

Add the widget supporting AnimationStyle in the docs

cc: @nate-thegrate

nate-thegrate · 2024-09-06T19:26:51Z

@TahaTesser thanks very much for this information!

I've created an issue to track progress on this feature: #154744

cc: @nploi

dkwingsmt · 2024-09-06T19:36:37Z

Thank you very much for this information! Learning something today!

flutter/flutter@45ef8f3...2e221e7 2024-09-06 [email protected] Fix DropdownMenu focused item styles (flutter/flutter#153159) 2024-09-06 [email protected] Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. (flutter/flutter#154048) 2024-09-06 [email protected] [tool] Add `dartFileName` setting for platform plugins (flutter/flutter#153099) 2024-09-06 [email protected] [Conductor] Add ability to override mirror, add tests for default arg parsing and custom arg parsing (flutter/flutter#154363) 2024-09-06 [email protected] Improve CupertinoPopupSurface appearance (flutter/flutter#151430) 2024-09-06 [email protected] Roll Packages from 71e827e to 56df73e (1 revision) (flutter/flutter#154725) 2024-09-06 [email protected] Quick access to style guide (flutter/flutter#154689) 2024-09-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (#154691)" (flutter/flutter#154726) 2024-09-05 [email protected] Improve iOS unpack target's error messages (flutter/flutter#154649) 2024-09-05 [email protected] Made some pixel tests fuzzy (flutter/flutter#154680) 2024-09-05 [email protected] Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (flutter/flutter#154691) 2024-09-05 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 7.0.0 to 7.0.1 (flutter/flutter#154690) 2024-09-05 [email protected] Normalize Dialog theme (flutter/flutter#153982) 2024-09-05 [email protected] iOS,macOS: Do not copy unsigned_binaries.txt to build outputs (flutter/flutter#154684) 2024-09-05 [email protected] Roll Flutter Engine from e042ff5df7af to c50eb8a65097 (1 revision) (flutter/flutter#154679) 2024-09-05 [email protected] Add proguard rule to keep the class for all implementations of FlutterPlugin (flutter/flutter#154677) 2024-09-05 [email protected] Fix DropdownMenu menu does not follow the text field (flutter/flutter#154667) 2024-09-05 [email protected] Roll Flutter Engine from a156e713f4dc to e042ff5df7af (1 revision) (flutter/flutter#154678) 2024-09-05 [email protected] Fix ZoomPageTransitionsBuilder hardcoded fill color (flutter/flutter#154057) 2024-09-05 [email protected] Roll Flutter Engine from 34b61eb53b99 to a156e713f4dc (1 revision) (flutter/flutter#154672) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

nploi · 2024-09-07T03:53:35Z

@TahaTesser thanks very much for this information!

I've created an issue to track progress on this feature: #154744

cc: @nploi

Thank you @nate-thegrate @TahaTesser, i will support AnimationStyle for show dialog methods.

can you assign me this #154744 @nate-thegrate ?

…inoDialogRoute` and show dialog methods. (flutter/flutter#154048)

flutter/flutter@45ef8f3...2e221e7 2024-09-06 [email protected] Fix DropdownMenu focused item styles (flutter/flutter#153159) 2024-09-06 [email protected] Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. (flutter/flutter#154048) 2024-09-06 [email protected] [tool] Add `dartFileName` setting for platform plugins (flutter/flutter#153099) 2024-09-06 [email protected] [Conductor] Add ability to override mirror, add tests for default arg parsing and custom arg parsing (flutter/flutter#154363) 2024-09-06 [email protected] Improve CupertinoPopupSurface appearance (flutter/flutter#151430) 2024-09-06 [email protected] Roll Packages from 71e827e to 56df73e (1 revision) (flutter/flutter#154725) 2024-09-06 [email protected] Quick access to style guide (flutter/flutter#154689) 2024-09-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (#154691)" (flutter/flutter#154726) 2024-09-05 [email protected] Improve iOS unpack target's error messages (flutter/flutter#154649) 2024-09-05 [email protected] Made some pixel tests fuzzy (flutter/flutter#154680) 2024-09-05 [email protected] Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (flutter/flutter#154691) 2024-09-05 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 7.0.0 to 7.0.1 (flutter/flutter#154690) 2024-09-05 [email protected] Normalize Dialog theme (flutter/flutter#153982) 2024-09-05 [email protected] iOS,macOS: Do not copy unsigned_binaries.txt to build outputs (flutter/flutter#154684) 2024-09-05 [email protected] Roll Flutter Engine from e042ff5df7af to c50eb8a65097 (1 revision) (flutter/flutter#154679) 2024-09-05 [email protected] Add proguard rule to keep the class for all implementations of FlutterPlugin (flutter/flutter#154677) 2024-09-05 [email protected] Fix DropdownMenu menu does not follow the text field (flutter/flutter#154667) 2024-09-05 [email protected] Roll Flutter Engine from a156e713f4dc to e042ff5df7af (1 revision) (flutter/flutter#154678) 2024-09-05 [email protected] Fix ZoomPageTransitionsBuilder hardcoded fill color (flutter/flutter#154057) 2024-09-05 [email protected] Roll Flutter Engine from 34b61eb53b99 to a156e713f4dc (1 revision) (flutter/flutter#154672) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…tinoDialogRoute` and show dialog methods. (#154048)" (#154743) Reverts: flutter/flutter#154048 Initiated by: nate-thegrate Reason for reverting: using `AnimationStyle` instead would allow for complete animation duration and curve customization. Original PR Author: nploi Reviewed By: {piedcipher, nate-thegrate} This change reverts the following previous change: Currently we don't support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods , This PR will to support that.

flutter/flutter@45ef8f3...2e221e7 2024-09-06 [email protected] Fix DropdownMenu focused item styles (flutter/flutter#153159) 2024-09-06 [email protected] Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. (flutter/flutter#154048) 2024-09-06 [email protected] [tool] Add `dartFileName` setting for platform plugins (flutter/flutter#153099) 2024-09-06 [email protected] [Conductor] Add ability to override mirror, add tests for default arg parsing and custom arg parsing (flutter/flutter#154363) 2024-09-06 [email protected] Improve CupertinoPopupSurface appearance (flutter/flutter#151430) 2024-09-06 [email protected] Roll Packages from 64aff14 to 5e19df0 (1 revision) (flutter/flutter#154725) 2024-09-06 [email protected] Quick access to style guide (flutter/flutter#154689) 2024-09-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (#154691)" (flutter/flutter#154726) 2024-09-05 [email protected] Improve iOS unpack target's error messages (flutter/flutter#154649) 2024-09-05 [email protected] Made some pixel tests fuzzy (flutter/flutter#154680) 2024-09-05 [email protected] Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (flutter/flutter#154691) 2024-09-05 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 7.0.0 to 7.0.1 (flutter/flutter#154690) 2024-09-05 [email protected] Normalize Dialog theme (flutter/flutter#153982) 2024-09-05 [email protected] iOS,macOS: Do not copy unsigned_binaries.txt to build outputs (flutter/flutter#154684) 2024-09-05 [email protected] Roll Flutter Engine from e042ff5df7af to c50eb8a65097 (1 revision) (flutter/flutter#154679) 2024-09-05 [email protected] Add proguard rule to keep the class for all implementations of FlutterPlugin (flutter/flutter#154677) 2024-09-05 [email protected] Fix DropdownMenu menu does not follow the text field (flutter/flutter#154667) 2024-09-05 [email protected] Roll Flutter Engine from a156e713f4dc to e042ff5df7af (1 revision) (flutter/flutter#154678) 2024-09-05 [email protected] Fix ZoomPageTransitionsBuilder hardcoded fill color (flutter/flutter#154057) 2024-09-05 [email protected] Roll Flutter Engine from 34b61eb53b99 to a156e713f4dc (1 revision) (flutter/flutter#154672) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Support custom transition duration for DialogRoute

2c5d0ee

github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages labels Aug 24, 2024

Piinks reviewed Aug 28, 2024

View reviewed changes

Add transitionDuration for CupertinoDialogRoute, show dialogs and wri…

52b4b27

…te unit-test

github-actions Bot added p: cupertino_ui cupertino_ui package in flutter/packages f: routes Navigator, Router, and related APIs. labels Aug 31, 2024

nploi changed the title ~~Support custom transition duration for DialogRoute~~ Support custom transition duration for DialogRoute, CupertinoDialogRoute and show dialogs. Aug 31, 2024

Merge branch 'master' into master

27e7aae

nploi requested a review from Piinks August 31, 2024 05:14

nploi changed the title ~~Support custom transition duration for DialogRoute, CupertinoDialogRoute and show dialogs.~~ Support custom transition duration for DialogRoute, CupertinoDialogRoute and show dialog methods. Aug 31, 2024

nploi added 4 commits August 31, 2024 12:17

Update unit-test

b77401e

Merge branch 'master' of https://github.com/nploi/flutter

f098172

Fix analyze

f8e3076

Merge branch 'master' into master

0f8e595

Piinks requested review from dkwingsmt and nate-thegrate September 4, 2024 18:20

nate-thegrate reviewed Sep 4, 2024

View reviewed changes

Comment thread packages/flutter/lib/src/material/dialog.dart Outdated

Comment thread packages/flutter/test/material/dialog_test.dart Outdated

Add default transition duration, update unit-test

0a94b86

nploi requested a review from nate-thegrate September 5, 2024 04:02

nate-thegrate approved these changes Sep 5, 2024

View reviewed changes

Comment thread packages/flutter/test/material/dialog_test.dart Outdated

Fix lint

ca89458

piedcipher reviewed Sep 5, 2024

View reviewed changes

Fix lint

3cbbda8

nploi requested a review from piedcipher September 5, 2024 15:44

dkwingsmt reviewed Sep 5, 2024

View reviewed changes

piedcipher approved these changes Sep 5, 2024

View reviewed changes

Fix lint

6b8b007

nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2024

auto-submit Bot merged commit 0eaeb0d into flutter:master Sep 6, 2024

engine-flutter-autoroll mentioned this pull request Sep 6, 2024

Roll Flutter from 45ef8f36ff41 to 2e221e7308ba (20 revisions) flutter/packages#7599

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2024

0eaeb0d Support custom transition duration for DialogRoute, `Cupert…

4ad3a2e

…inoDialogRoute` and show dialog methods. (flutter/flutter#154048)

nate-thegrate added the revert Autorevert PR (with "Reason for revert:" comment) label Sep 6, 2024

This comment was marked as resolved.

Sign in to view

auto-submit Bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Sep 6, 2024

nate-thegrate added the revert Autorevert PR (with "Reason for revert:" comment) label Sep 6, 2024

auto-submit Bot pushed a commit that referenced this pull request Sep 6, 2024

Revert "Support custom transition duration for DialogRoute, `Cupert…

1afed1d

…inoDialogRoute` and show dialog methods. (#154048)" This reverts commit 0eaeb0d.

auto-submit Bot mentioned this pull request Sep 6, 2024

Reverts "Support custom transition duration for DialogRoute, CupertinoDialogRoute and show dialog methods. (#154048)" #154743

Merged

9 tasks done

auto-submit Bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Sep 6, 2024

nate-thegrate added the was reverted (optionally) marks a pull request as having been reverted after it was merged label Sep 6, 2024

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024

0eaeb0d Support custom transition duration for DialogRoute, `Cupert…

80e8bc6

…inoDialogRoute` and show dialog methods. (flutter/flutter#154048)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024

0eaeb0d Support custom transition duration for DialogRoute, `Cupert…

1ad90a3

…inoDialogRoute` and show dialog methods. (flutter/flutter#154048)

	Duration? transitionDuration = _kCupertinoDialogRouteTransitionDuration,
	Duration? transitionDuration,

	transitionDuration: transitionDuration ?? _kDialogRouteTransitionDuration,
	transitionDuration: transitionDuration,

Conversation

nploi commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard Bot commented Aug 24, 2024

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

nploi commented Aug 31, 2024

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

piedcipher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nploi commented Sep 5, 2024

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

piedcipher left a comment

Choose a reason for hiding this comment

Uh oh!

nploi commented Sep 6, 2024

Uh oh!

nate-thegrate commented Sep 6, 2024

Uh oh!

TahaTesser commented Sep 6, 2024

Uh oh!

TahaTesser commented Sep 6, 2024

Uh oh!

TahaTesser commented Sep 6, 2024

Uh oh!

This comment was marked as resolved.

nate-thegrate commented Sep 6, 2024

Uh oh!

TahaTesser commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nate-thegrate commented Sep 6, 2024

Uh oh!

dkwingsmt commented Sep 6, 2024

Uh oh!

nploi commented Sep 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

nploi commented Aug 24, 2024 •

edited

Loading

TahaTesser commented Sep 6, 2024 •

edited

Loading