Added parallax effect for cupertino fullscreenDialog #50180

Kavantix · 2020-02-05T11:29:09Z

Description

Added the slideTransition for the parallax effect when a route was pushed as fullscreenDialog.

Related Issues

Fixes #20073

Tests

Added test that check that the parallax effect is working and is not used if a fullscreen routes is push over.

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.

[ x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
[x ] I signed the CLA.
[x ] I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
[ x] I read the Tree Hygiene wiki page, which explains my responsibilities.
[ x] I updated/added relevant documentation (doc comments with ///).
[x ] All existing and new tests are passing.
[x ] The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
[x ] I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

[x ] No, no existing tests failed, so this is not a breaking change.

xster · 2020-03-11T18:07:53Z

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

+        //
+        // In the middle of a back gesture drag, let the transition be linear to
+        // match finger motions.
+        linearTransition: isPopGestureInProgress(route),


I suspect this might not be needed. Can you find a native example in iOS where a fullscreen modal page is back swipable?

Indeed the primaryanimation does not use this, however, the secondary animation does still need it

ah you're right, thanks

xster · 2020-03-11T18:11:13Z

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

@@ -424,27 +431,47 @@ class CupertinoFullscreenDialogTransition extends StatelessWidget {
  /// Creates an iOS-style transition used for summoning fullscreen dialogs.
  CupertinoFullscreenDialogTransition({
    Key key,
-    @required Animation<double> animation,


we're renaming an existing public API but I'm surprised this didn't break any tests 😅
given our latest breaking change policy, it means this one's good to go https://flutter.dev/docs/resources/compatibility

Yeah this part of flutter isnt very well tested to be honest

xster · 2020-03-11T18:12:05Z

packages/flutter/test/cupertino/route_test.dart

@@ -502,6 +502,165 @@ void main() {
    expect(tester.getTopLeft(find.byType(Placeholder)).dy, closeTo(600.0, 0.1));
  });

+  testWidgets('Bottom route has parallax when non fullscreen route is pushed on top', (WidgetTester tester) async {


fullscreen dialog route has parallax exit animation when non fullscreen route is pushed on top

This test is not targeting full screen specifically, also its not only the exit animation that has parallax
So maybe: CupertinoPageRoute has parallax animation when non fullscreenDialog route is pushed on top

xster · 2020-03-11T18:13:44Z

packages/flutter/test/cupertino/route_test.dart

+                CupertinoButton(
+                  child: const Text('Button'),
+                  onPressed: () {
+                    Navigator.push<void>(context, CupertinoPageRoute<void>(


you're not testing the new functionality on CupertinoFullscreenDialogTransition you created right? That would involve 2 routes with the bottom one having fullscreenDialog true

You’re correct, it seems I forgot to also test that explicitely.
This test is still needed though since it was missing.
Is it considered bad or good practice to move the body of this test into a function that gets an argument? (Whether the bottomroute should be fullscreenDialog)

having helper functions is ok

Kavantix · 2020-03-11T21:29:09Z

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

+        //
+        // In the middle of a back gesture drag, let the transition be linear to
+        // match finger motions.
+        linearTransition: isPopGestureInProgress(route),


Indeed the primaryanimation does not use this, however, the secondary animation does still need it

Kavantix · 2020-03-11T21:30:03Z

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

@@ -424,27 +431,47 @@ class CupertinoFullscreenDialogTransition extends StatelessWidget {
  /// Creates an iOS-style transition used for summoning fullscreen dialogs.
  CupertinoFullscreenDialogTransition({
    Key key,
-    @required Animation<double> animation,


Yeah this part of flutter isnt very well tested to be honest

Kavantix · 2020-03-11T21:31:53Z

packages/flutter/test/cupertino/route_test.dart

@@ -502,6 +502,165 @@ void main() {
    expect(tester.getTopLeft(find.byType(Placeholder)).dy, closeTo(600.0, 0.1));
  });

+  testWidgets('Bottom route has parallax when non fullscreen route is pushed on top', (WidgetTester tester) async {


This test is not targeting full screen specifically, also its not only the exit animation that has parallax
So maybe: CupertinoPageRoute has parallax animation when non fullscreenDialog route is pushed on top

packages/flutter/test/cupertino/route_test.dart

Kavantix · 2020-03-11T21:42:16Z

packages/flutter/test/cupertino/route_test.dart

+                CupertinoButton(
+                  child: const Text('Button'),
+                  onPressed: () {
+                    Navigator.push<void>(context, CupertinoPageRoute<void>(


You’re correct, it seems I forgot to also test that explicitely.
This test is still needed though since it was missing.
Is it considered bad or good practice to move the body of this test into a function that gets an argument? (Whether the bottomroute should be fullscreenDialog)

Kavantix · 2020-03-17T06:15:58Z

@xster I updated the names of the test, added that both fullscreen dialog and non fullscreen dialog cases are tested and updated the comments such that they reflect which animations are affected by the linearTransition boolean.
Turns out that the comment on CupertinoPageTransition said that only the primary animation would be affected but that was not true so also updated that one.

xster

LGTM. Thanks for your patience!

jamesderlin · 2020-04-07T09:09:27Z

Someone complained about the breakage: https://stackoverflow.com/questions/61075440/after-latest-flutter-upgrade-project-is-not-building

While I understand that this isn't strictly considered breaking according to the breaking change policy, in this case it's clearly from a complete lack of testing. A @required named parameter was changed; anyone using CupertinoFullscreenDialogTransition cannot avoid being broken. This should fall under:

In cases where we can imagine reasonable scenarios where developers would be affected negatively, by courtesy, once the change has landed, engineers are encouraged to announce the changes by sending an e-mail to flutter-announce and listing it on our Changelog wiki page (such that they will be included in our release notes)

xster · 2020-04-07T18:38:17Z

It looks like we broke the extended_images pub package https://github.com/fluttercandies/extended_image/blob/f65ce8f5dd37b3d54d14422adf31e40a98c435f0/lib/src/gesture/extended_image_slide_page_route.dart#L331.

@Kavantix would you like to create an announcement message and send it to flutter-announce? I'll help you approve the group message. See https://github.com/flutter/website/blob/master/src/docs/release/breaking-changes/template.md for a message template.

I'll add this change to the changelog.

Kavantix · 2020-04-08T16:47:41Z

Sure I’ll do that.

Shouldn’t the changelog also reflect that two other required parameters where added @xster?

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Feb 5, 2020

googlebot added the cla: yes label Feb 5, 2020

Kavantix requested a review from xster February 5, 2020 11:29

Kavantix requested a review from HansMuller February 13, 2020 13:32

jmagman added this to Awaiting triage in iOS Platform - Cupertino widget review Feb 25, 2020

xster reviewed Mar 11, 2020

View reviewed changes

Kavantix commented Mar 11, 2020

View reviewed changes

Kavantix force-pushed the cupertino-fullscreen-transition-parallax branch from 58ce152 to af12d72 Compare March 17, 2020 06:09

Kavantix added 2 commits March 17, 2020 07:10

Added test parallax effect of CupertinoRoute

35b88bc

Added parallax effect for cupertino fullscreenDialog

e78fc94

Kavantix force-pushed the cupertino-fullscreen-transition-parallax branch from af12d72 to e78fc94 Compare March 17, 2020 06:10

Piinks added the a: fidelity Matching the OEM platforms better label Mar 18, 2020

Removed trailing whitespace

1935c40

Kavantix force-pushed the cupertino-fullscreen-transition-parallax branch from 2684065 to 1935c40 Compare March 23, 2020 15:02

xster approved these changes Mar 28, 2020

View reviewed changes

xster added the waiting for tree to go green label Mar 28, 2020

fluttergithubbot merged commit 6c9071d into flutter:master Mar 28, 2020

iOS Platform - Cupertino widget review automation moved this from Awaiting triage to Engineer reviewed Mar 28, 2020

narcodico mentioned this pull request May 7, 2020

After update flutter 1.17.0 getting error fluttercandies/extended_image#179

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added parallax effect for cupertino fullscreenDialog #50180

Added parallax effect for cupertino fullscreenDialog #50180

Kavantix commented Feb 5, 2020

xster Mar 11, 2020

Kavantix Mar 11, 2020 •

edited

xster Mar 28, 2020

xster Mar 11, 2020

Kavantix Mar 11, 2020

xster Mar 11, 2020

Kavantix Mar 11, 2020

xster Mar 11, 2020

Kavantix Mar 11, 2020 •

edited

xster Mar 28, 2020

Kavantix Mar 11, 2020 •

edited

Kavantix Mar 11, 2020

Kavantix Mar 11, 2020

Kavantix Mar 11, 2020 •

edited

Kavantix commented Mar 17, 2020

xster left a comment

jamesderlin commented Apr 7, 2020

xster commented Apr 7, 2020

Kavantix commented Apr 8, 2020

Added parallax effect for cupertino fullscreenDialog #50180

Added parallax effect for cupertino fullscreenDialog #50180

Conversation

Kavantix commented Feb 5, 2020

Description

Related Issues

Tests

Checklist

Breaking Change

Choose a reason for hiding this comment

Kavantix Mar 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kavantix Mar 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kavantix Mar 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kavantix Mar 11, 2020 • edited

Choose a reason for hiding this comment

Kavantix commented Mar 17, 2020

xster left a comment

Choose a reason for hiding this comment

jamesderlin commented Apr 7, 2020

xster commented Apr 7, 2020

Kavantix commented Apr 8, 2020

Kavantix Mar 11, 2020 •

edited

Kavantix Mar 11, 2020 •

edited

Kavantix Mar 11, 2020 •

edited

Kavantix Mar 11, 2020 •

edited