Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. #154048

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!

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

packages/flutter/test/material/dialog_test.dart

nate-thegrate

LGTM, thanks for making this improvement!

packages/flutter/test/material/dialog_test.dart

piedcipher

some smol clean-up

packages/flutter/test/material/dialog_test.dart

packages/flutter/lib/src/cupertino/route.dart

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

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

packages/flutter/lib/src/cupertino/route.dart

  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

packages/flutter/lib/src/cupertino/route.dart

    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

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

    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

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

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


As suggested above, add to doc.

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

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

    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-05T19:03:58Z

hi @dkwingsmt @piedcipher, Thank for your reviews. I just added docs, using nullable value and write a test for RawDialogRoute.transitionDuration, pls help to check it.

nate-thegrate · 2024-09-06T00:03:38Z

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

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 leroux_bruno@yahoo.fr Fix DropdownMenu focused item styles (flutter/flutter#153159) 2024-09-06 phucloi.nguyen@manabie.com Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. (flutter/flutter#154048) 2024-09-06 51940183+Sameri11@users.noreply.github.com [tool] Add `dartFileName` setting for platform plugins (flutter/flutter#153099) 2024-09-06 reidbaker@google.com [Conductor] Add ability to override mirror, add tests for default arg parsing and custom arg parsing (flutter/flutter#154363) 2024-09-06 59215665+davidhicks980@users.noreply.github.com Improve CupertinoPopupSurface appearance (flutter/flutter#151430) 2024-09-06 engine-flutter-autoroll@skia.org Roll Packages from 71e827e to 56df73e (1 revision) (flutter/flutter#154725) 2024-09-06 goderbauer@google.com 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 737941+loic-sharma@users.noreply.github.com Improve iOS unpack target's error messages (flutter/flutter#154649) 2024-09-05 30870216+gaaclarke@users.noreply.github.com Made some pixel tests fuzzy (flutter/flutter#154680) 2024-09-05 engine-flutter-autoroll@skia.org 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 36861262+QuncCccccc@users.noreply.github.com Normalize Dialog theme (flutter/flutter#153982) 2024-09-05 chris@bracken.jp iOS,macOS: Do not copy unsigned_binaries.txt to build outputs (flutter/flutter#154684) 2024-09-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from e042ff5df7af to c50eb8a65097 (1 revision) (flutter/flutter#154679) 2024-09-05 34871572+gmackall@users.noreply.github.com Add proguard rule to keep the class for all implementations of FlutterPlugin (flutter/flutter#154677) 2024-09-05 bruno.leroux@gmail.com Fix DropdownMenu menu does not follow the text field (flutter/flutter#154667) 2024-09-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from a156e713f4dc to e042ff5df7af (1 revision) (flutter/flutter#154678) 2024-09-05 103767860+dy0gu@users.noreply.github.com Fix ZoomPageTransitionsBuilder hardcoded fill color (flutter/flutter#154057) 2024-09-05 engine-flutter-autoroll@skia.org 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 bmparr@google.com,rmistry@google.com,stuartmorgan@google.com 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)

Support custom transition duration for DialogRoute

2c5d0ee

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. 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 f: cupertino flutter/packages/flutter/cupertino repository 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

packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/dialog_test.dart Outdated Show resolved Hide resolved

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

packages/flutter/test/material/dialog_test.dart Outdated Show resolved Hide resolved

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

nploi requested a review from dkwingsmt September 5, 2024 19:04

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

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,

Support custom transition duration for DialogRoute, CupertinoDialogRoute and show dialog methods. #154048

Support custom transition duration for DialogRoute, CupertinoDialogRoute and show dialog methods. #154048

Uh oh!

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 5, 2024

Uh oh!

nate-thegrate commented Sep 6, 2024

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

Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. #154048

Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. #154048

nploi commented Aug 24, 2024 •

edited

Loading

TahaTesser commented Sep 6, 2024 •

edited

Loading