-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Add CupertinoSliverNavigationBar large title magnification on over scroll #72100
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 #72100
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@xster I realized I forgot to add a min scale of 1.0, so the large title was ever so slightly scaling down when scrolling up. Fixed now. |
xster
left a comment
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. This is ready for tests.
| maxLines: 1, | ||
| overflow: TextOverflow.ellipsis, | ||
| child: components.largeTitle!, | ||
| // `LayoutBuilder` provides the height of the nav bar for the large |
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.
A LayoutBuilder lets us figure out the height of the nav bar after stretching from overscrolls. This lets us determine how much to grow the size of the large title text during overscrolls.
| // The `constraints.maxHeight` value is the height of the nav bar, | ||
| // and `maxExtent` is the default large title height the nav bar snaps back to. | ||
| // The difference between the two heights is used to scale the title. | ||
| scale: math.max(1.0, math.min(1.15, 1 + (constraints.maxHeight - maxExtent) / maxExtent * 0.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.
This line is longer than 80 characters. Line breaking at strategic points may help readability too.
| // The `constraints.maxHeight` value is the height of the nav bar, | ||
| // and `maxExtent` is the default large title height the nav bar snaps back to. | ||
| // The difference between the two heights is used to scale the title. | ||
| scale: math.max(1.0, math.min(1.15, 1 + (constraints.maxHeight - maxExtent) / maxExtent * 0.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.
super nit: the logic is probably easier to see with clamp: (1.0 + (constraints.maxHeight - maxExtent) / maxExtent * 0.12)).clamp(1.0, 1.15),
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.
Is the maxExtent here supposed to be minExtent? When constraints.maxHeight == minExtent the expression should evaluate to 1.0 from the looks of it?
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.
Will this have hit testing problems when we scale the title? oh nvm the container resizes.Transform.scale doesn't seem to change the layout size of the render object, so I assume when you change the scale, the maximum hit testing region is still the size of the render object
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.
it's just a graphical effect. It's not interactive and you can't keep it in the transient overscroll state.
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.
Is it possible for an overlength title to get clipped or overlap the trailing widget?
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.
Very good point. Worth checking what native iOS does when the large title is already horizontally long enough to almost overflow.
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.
Hm. Good point.
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.
Ok this is what I understand from the behavior:
The trailing widget in large title nav bars lies above the title, therefore it won't overlap horizontally. Because scaling only happens when the height of the nav bar increases, it can't grow large enough vertically to overlap the navigation items (trailing, leading.)
The title may scale up to 1.15, but it is bound to its horizontal margins. Meaning if the text is almost large enough to overflow or if it is truncated, the title text may ever so slightly scale up to the horizontal margins. If it is already exactly at its horizontal margins, it will not scale. I'll update the PR to handle this edge case.
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.
@xster @LongCatIsLooong I'm having trouble setting the max-width condition for the scale transformation. My original thought was to have a condition that would check for the width of the large title and ensure it's less than the nav bar builder's constraints.maxWidth, but I can't find a way to get the width of the large title text itself. Any ideas?
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.
ah doh, sorry I missed this somehow.
Yes, it's somewhat intentionally complicated because text layout is expensive, so the API is rather low level. There are 2 ways.
The canonical way is to use a RenderBox the way the FittedBox object uses a RenderBox. During the layout phase, it lets its child calculate its size then reacts accordingly in its own paint.
The slightly less complicated way is to "dry run" it using a TextPainter. There's a bit of that going on in the picker.
|
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. |
|
Any updates on this? @DanielEdrisian |
|
Since no progress has been made here for a while I'm going to close this PR to remove it from our review queue, but please do not hesitate to create a new PR if you have a chance to add tests! Thanks! |
Description
Per #62298, adds magnification for
CupertinoSliverNavigationBarlarge title on over scroll.Related Issues
Original issue: #62298
[Merged] Stretch nav bar on over-scroll PR: #71707
Tests
No tests yet.
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.