Skip to content

Conversation

@DanielEdrisian
Copy link
Contributor

Description

Per #62298, adds magnification for CupertinoSliverNavigationBar large 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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • 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.

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Dec 10, 2020
@flutter-dashboard
Copy link

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.

@DanielEdrisian
Copy link
Contributor Author

@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.

Copy link
Member

@xster xster left a 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
Copy link
Member

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)),
Copy link
Member

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)),
Copy link
Contributor

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),

Copy link
Contributor

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?

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Dec 11, 2020

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? 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 oh nvm the container resizes.

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Good point.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@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.

@Trulsmatias
Copy link

Any updates on this? @DanielEdrisian

@Hixie
Copy link
Contributor

Hixie commented Nov 17, 2021

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!

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

Labels

f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants