Skip to content
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

Support floating the header slivers of a NestedScrollView #59187

Merged
merged 27 commits into from
Jun 12, 2020

Conversation

Piinks
Copy link
Contributor

@Piinks Piinks commented Jun 10, 2020

Description

Updated PR based on #57707

This PR adds support for floating the outer header slivers of a NestedScrollView over the inner scroll view. 🎉

This is achieved by introducing NestedScrollView.floatHeaderSlivers, a boolean flag for specifying that the header is expected to float. When true, the NestedScrollView's scrolling coordinator will prioritize the outer scroll view before passing any leftover delta to the inner scroll view.
Normally, when an app bar floats, the render object keeps track of a mock scroll offset (_effectiveScrollOffset), since as the app bar moves in and out of view, its position can vary in accordance with the actual scroll offset. Instead of following the mocking convention, as a separate scroll view, we can just change the real offset when floating from a NestedScrollView.

What is different from #57707 is that the nestedSnap feature has been removed. The solution proposed in #57707 was unfavorable, so instead this documents that floating-snapping SliverAppBars are not supported in the NestedScrollView. You can snap or float, not both. I have filed a issue for this limitation: #59189

There is documentation and sample applications to illustrate use cases in conjunction with the many configurations of a sliver app bar. This change does not address stretching sliver app bars. That is being tracked in #54059, and will come in a follow up change.

Finally, this also makes a small change to the SliverGeometry.maxScrollObstruction of the RenderSliverFloatingPinnedPersistentHeader. The previous geometry set the maxExtent, when it should be the minExtent. Using the maxExtent prevents an expanded, pinned, floating app bar from collapsing as expected.

Related Issues

Fixes #49560
Fixes #29264
Fixes #17518
Fixes #20877

Related change: #55069

Tests

I added the following tests:

  • NestedScrollView can float outer sliver with inner scroll view:
    • float
    • float expanded
    • only snap
    • only snap expanded
    • float pinned
    • float pinned expanded

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.

  • No, no existing tests failed, so this is not a breaking change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 10, 2020
@Piinks Piinks requested a review from goderbauer June 10, 2020 19:14
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM, just some doc nit picking...

packages/flutter/lib/src/widgets/nested_scroll_view.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/nested_scroll_view.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/nested_scroll_view.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/nested_scroll_view.dart Outdated Show resolved Hide resolved
/// ```
/// {@end-tool}
///
/// ## More on using [SliverAppBar]s with [NestedScrollView]s
Copy link
Member

Choose a reason for hiding this comment

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

High-Level comment: Reading this entire section, I am wondering if we can shorten this a little bit. Instead of giving longer explanations referring to the internals of the nested scroll view, maybe we can just say in each section whether that type of app bar does or doesn't work out of the box as expected. And if it doesn't work out of the box mention how the scrollview needs to be configured to make it work (the code examples should be left in, of course!).

Copy link
Contributor Author

@Piinks Piinks Jun 11, 2020

Choose a reason for hiding this comment

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

I go back and forth on this. I think it's important to explain a lot of these things, as we often have issues filed that are resolved by just explaining oh you need the SliverOverlap* widgets etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can never have too much documentation, right? :)

@Piinks Piinks requested a review from goderbauer June 11, 2020 23:44
@Piinks Piinks added customer: crowd Affects or could affect many people, though not necessarily a specific customer. customer: quill (g3) f: scrolling Viewports, list views, slivers, etc. a: annoyance Repeatedly frustrating issues with non-experimental functionality a: quality A truly polished experience d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation labels Jun 11, 2020
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

Still LGTM

@Piinks
Copy link
Contributor Author

Piinks commented Jun 12, 2020

Still LGTM

Sweet!

@fluttergithubbot fluttergithubbot merged commit 64f42c0 into flutter:master Jun 12, 2020
zljj0818 pushed a commit to zljj0818/flutter that referenced this pull request Jun 22, 2020
@HeckYeah-01
Copy link

@Piinks , I would like to thank you so much for your effort on this feature, just have a question when I moved into this I lost the position.pixels of the CustomScrollView inside the nested because you know I removed the controller from it and kept the nested.

I am trying to save the position of the scroll after every restart,

I really appreciate your help

@TheAnimatrix
Copy link

How do i get the HeaderBuilder to build a persistent header on top of my NestedScrollView body. I'm trying to get a transparent effect and there seems to be nothing under the header. ( looks like the inner scroll view is always translated to not fall below it )

zmtzawqlp added a commit to fluttercandies/extended_nested_scroll_view that referenced this pull request Jul 7, 2020
* Merge flutter/flutter#59187(Support floating the header slivers of a NestedScrollView)
zmtzawqlp added a commit to fluttercandies/extended_nested_scroll_view that referenced this pull request Jul 7, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: annoyance Repeatedly frustrating issues with non-experimental functionality a: quality A truly polished experience customer: crowd Affects or could affect many people, though not necessarily a specific customer. customer: quill (g3) d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
6 participants