-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Add CupertinoSliverNavigationBar
large title magnification on over scroll
#110127
Conversation
As you mentioned in the pull request description, all bets are off if |
After some tests, I thought that the best way to achieve magnification effect of the large title is to extend the @LongCatIsLooong what do you think about this approach? |
based on your current implementation (calculating the scale based on |
That's why I wrapped child in the |
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Sorry for the delay. Some of the failing tests seem to be testing the right thing:
|
Right, I just updated the code and everything seems to be passing now. |
BTW, could anyone review flutter/platform_tests#12? |
There was a problem hiding this 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!
? 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@Hixie @MitchellGoodwin could you take another look at this PR? |
There was a problem hiding this 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.
examples/api/test/cupertino/nav_bar/cupertino_sliver_nav_bar.0_test.dart
Outdated
Show resolved
Hide resolved
Whoops, I accidentally merged instead of rebasing, but all checks passed nevertheless. |
…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
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:
Fixes #62298
![](https://user-images.githubusercontent.com/57679865/186196035-bbc5c8a3-836f-46ea-ae9d-6af67083ab8d.gif)
![](https://user-images.githubusercontent.com/57679865/186195797-055f08bf-df15-498d-91e9-d1bb6a0a8b8f.gif)
![](https://user-images.githubusercontent.com/57679865/191864469-a000a016-bb6c-4ef1-92e8-39b04c651e78.gif)
Pre-launch Checklist
///
).