Skip to content

Add CupertinoSliverNavigationBar large title magnification on over scroll #110127

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 32 commits into from
Dec 8, 2022
Merged

Add CupertinoSliverNavigationBar large title magnification on over scroll #110127

merged 32 commits into from
Dec 8, 2022
+294 −34

Conversation

ivirtex
Copy link
Contributor

@ivirtex ivirtex commented Aug 24, 2022 •
edited
Loading

Per #62298, adds magnification for CupertinoSliverNavigationBar large title on over scroll.
This is a continuation of my previous PR #103299, but I addressed comments and added more tests.

I decided to make use of TextPainter to calculate the width of the text and set the maximum scale appropriately,
I don't know if that's good enough because clipping will occur when the child widget is different from Text.

Also, when testing how native iOS handles this edge case, I noticed that padding applied to the large text is wrong (line 830).
This causes text to stick to the side of the screen before truncating (can be seen on the second gif).
To match the native iOS look, padding should also be applied to the end property of EdgeInsetsDirectional.only:

Padding(
  padding: const EdgeInsetsDirectional.only(
    start: _kNavBarEdgePadding,
    end: _kNavBarEdgePadding,
    bottom: 8.0, // Bottom has a different padding.
),

Fixes #62298


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].
  • All existing and new tests are passing.

Sorry, something went wrong.

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Aug 24, 2022
@ivirtex ivirtex deleted the branch flutter:master August 25, 2022 14:09
@ivirtex ivirtex closed this Aug 25, 2022
@ivirtex ivirtex deleted the master branch August 25, 2022 14:09
@ivirtex ivirtex restored the master branch August 25, 2022 14:09
@ivirtex ivirtex reopened this Aug 25, 2022
@LongCatIsLooong
Copy link
Contributor

As you mentioned in the pull request description, all bets are off if largeTitle.child is not a Text widget, so I think a different method for measuring the size of the child widget is needed, or alternatively you could change the constraints to prevent child from overflowing its container.

@ivirtex
Copy link
Contributor Author

ivirtex commented Sep 11, 2022 •
edited
Loading

After some tests, I thought that the best way to achieve magnification effect of the large title is to extend the SingleChildRenderObjectWidget and apply the transformation just before painting.
This allows us to calculate the magnification scale on the first pass, avoiding any dry-runs.
Everything seems to work fine, magnification is correctly applied on the LTR and RTL text directions and no widget get clipped at the edge.

@LongCatIsLooong what do you think about this approach?

@LongCatIsLooong
Copy link
Contributor

based on your current implementation (calculating the scale based on size.width), I think scale is probably always going to be 1.0, because the child widget is going to take as much space as possible.

@ivirtex
Copy link
Contributor Author

ivirtex commented Sep 12, 2022

based on your current implementation (calculating the scale based on size.width), I think scale is probably always going to be 1.0, because the child widget is going to take as much space as possible.

That's why I wrapped child in the Align widget, so it takes only the space it needs. I am not sure if that's the best way to handle this, but it works.

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@LongCatIsLooong
Copy link
Contributor

Sorry for the delay. Some of the failing tests seem to be testing the right thing:

  • the "Media padding is applied to CupertinoSliverNavigationBar" is failing because _scale is greater than 1.0 when the scroll view isn't even scrolled. Maybe we should replace the child.size.height in 1.0 + (constraints.maxHeight - child.size.height) / child.size.height * 0.03 with something else. From the looks of it, it should be _kNavBarLargeTitleHeightExtension - 8.0?
  • It seems safe to just update the value from "44 - 8" to "44" from the Small title can be overridden test. The large title should have a height of 0 at that point.

@ivirtex
Copy link
Contributor Author

ivirtex commented Nov 28, 2022

  • the "Media padding is applied to CupertinoSliverNavigationBar" is failing because _scale is greater than 1.0 when the scroll view isn't even scrolled. Maybe we should replace the child.size.height in 1.0 + (constraints.maxHeight - child.size.height) / child.size.height * 0.03 with something else. From the looks of it, it should be _kNavBarLargeTitleHeightExtension - 8.0?
  • It seems safe to just update the value from "44 - 8" to "44" from the Small title can be overridden test. The large title should have a height of 0 at that point.

Right, I just updated the code and everything seems to be passing now.
I will make some side-to-side comparison tomorrow to check whether our 0.03 coefficient needs some tweaking.

@ivirtex
Copy link
Contributor Author

ivirtex commented Nov 29, 2022

0.03 seems to be pretty good for me.

BTW, could anyone review flutter/platform_tests#12?

LongCatIsLooong
LongCatIsLooong approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking the time to implement this and answer my questions!

packages/flutter/lib/src/cupertino/nav_bar.dart Outdated
? clampDouble(constraints.maxWidth / child.size.width, 1.0, 1.1)
: 1.1;
// The coefficient 0.03 may need some tweaking.
_scale = clampDouble(1.0 + (constraints.maxHeight - (_kNavBarLargeTitleHeightExtension - 8)) / (_kNavBarLargeTitleHeightExtension - 8) * 0.03, 1.0, maxScale);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make the 8 a private constant in the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@LongCatIsLooong
Copy link
Contributor

@Hixie @MitchellGoodwin could you take another look at this PR?

MitchellGoodwin
MitchellGoodwin approved these changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you for putting this together, it will be a great addition. For resolving the conflicts, can you rebase instead of merging? It may unstick the google testing.

@ivirtex
Copy link
Contributor Author

ivirtex commented Dec 8, 2022

Whoops, I accidentally merged instead of rebasing, but all checks passed nevertheless.

@ivirtex ivirtex requested a review from Hixie December 8, 2022 12:50
@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 8, 2022
@auto-submit auto-submit bot merged commit ef40e3e into flutter:master Dec 8, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2022
@engine-flutter-autoroll
ef40e3e Add CupertinoSliverNavigationBar large title magnification …
8026b55 
…on over scroll (flutter/flutter#110127)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2022
@engine-flutter-autoroll
ef40e3e Add CupertinoSliverNavigationBar large title magnification …
ec0681b 
…on over scroll (flutter/flutter#110127)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 8, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2022
@engine-flutter-autoroll
ef40e3e Add CupertinoSliverNavigationBar large title magnification …
45eb9ba 
…on over scroll (flutter/flutter#110127)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 9, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Dec 9, 2022
@engine-flutter-autoroll
ef40e3e Add CupertinoSliverNavigationBar large title magnification …
851582e 
…on over scroll (flutter/flutter#110127)
@ivirtex ivirtex mentioned this pull request Dec 13, 2022
Closed
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@ivirtex @gspencergoog
Add CupertinoSliverNavigationBar large title magnification on over …
26e18c4 
…scroll (flutter#110127)

* Add magnification of CupertinoSliverNavigationBar large title

* Fix padding in maximum scale computation

* Apply magnification by using RenderBox

* Do not pass key to the superclass constructor

* Use `clampDouble` instead of `clamp` extension method

* Remove trailing whitespaces to make linter happy

* Name test variables more precisely

* Move transform computation to `performLayout` and implement `hitTestChildren`

* Address comments

* Address comments

* Address comments

* Update comment about scale

* Fix hit-testing

* Fix hit-testing again

* Make linter happy

* Implement magnifying without using LayoutBuilder

* Remove trailing spaces

* Add hit-testing of the large title

* Remove whitespaces

* Fix scale computation and some tests

* Fix remaining tests

* Refactor and fix comments

* Update comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LongCatIsLooong LongCatIsLooong

@MitchellGoodwin MitchellGoodwin

@Hixie Hixie

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Strech animation for CupertinoSliverNavigationBar
4 participants
@ivirtex @LongCatIsLooong @Hixie @MitchellGoodwin