Navigation Menu

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

Jump to bottom

SliverFillRemaining flag for different use cases #33627

Merged
merged 12 commits into from Jun 14, 2019
Merged

SliverFillRemaining flag for different use cases #33627

merged 12 commits into from Jun 14, 2019

Conversation

Piinks
Copy link
Contributor

@Piinks Piinks commented May 30, 2019 •
edited

Description

This PR adds a flag to SliverFillRemaining to specify two different behaviors.

  1. In SliverFillRemaining fill more than remaining space available  #17444, the expected behavior is that the sliver will fill the remainder of the viewport and no further.
  2. However, when utilized within NestedScrollView, the sliver is expected to scroll beyond the viewport.

This flag, hasScrollBody, defaults to the behavior described in the latter example, maintaining the current behavior of NestedScrollView.
To achieve the desired behavior for the example in #17444, set the hasScrollBody property to be false.

See API Docs for SliverFillRemaining

Related Issues

Fixes #17444

Tests

  • SliverFillRemaining does not extend past viewport (when hasScrollBody set to false)
  • SliverFillRemaining scrolls beyond viewport by default

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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • No, this is not a breaking change.

@Piinks Piinks added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels May 30, 2019
@Piinks Piinks mentioned this pull request May 30, 2019
Closed
@Piinks
Copy link
Contributor Author

Piinks commented May 31, 2019

cc/ @goderbauer @dnfield
We discussed a few different ways to implement this offline, this being the most recent.
Also, in this case, I think it may be a breaking change...

@dnfield
Copy link
Contributor

dnfield commented May 31, 2019

Could we make this not be a breaking change (or a less breaking change?) by defaulting the flag to be the existing behavior?

goderbauer
goderbauer reviewed May 31, 2019
View reviewed changes
packages/flutter/lib/src/rendering/sliver_fill.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/sliver_fill.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/sliver_fill.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/sliver_fill.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/sliver.dart Outdated Show resolved Hide resolved
packages/flutter/test/widgets/nested_scroll_view_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/widgets/nested_scroll_view_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/widgets/nested_scroll_view_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/widgets/nested_scroll_view_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/widgets/sliver_fill_remaining_test.dart Outdated Show resolved Hide resolved
@goderbauer
Copy link
Member

Cirrus seems unhappy :(

@Piinks
Copy link
Contributor Author

Piinks commented Jun 3, 2019 •
edited

Could we make this not be a breaking change (or a less breaking change?) by defaulting the flag to be the existing behavior?

I have changed this so that the default behavior stays true to its current behavior.

I don't think that it will be a breaking change in this case.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 4, 2019
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 4, 2019
@Piinks Piinks self-assigned this Jun 12, 2019
goderbauer
goderbauer approved these changes Jun 14, 2019
View reviewed changes
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

@Piinks Piinks merged commit 81bbd3e into flutter:master Jun 14, 2019
tango5614 added a commit to tango5614/flutter that referenced this pull request Jun 14, 2019
@tango5614
Merge branch 'master' into fix28096
51b43c2 
* master: (24 commits)
  [flutter_tool,fuchsia] Update the install flow for packaging migration. (flutter#34447)
  SliverFillRemaining flag for different use cases (flutter#33627)
  SizedBox documentation (flutter#34424)
  Change API doc link to api.dart.dev (flutter#34388)
  2589785 Roll src/third_party/skia 87e885038893..c3252a04b377 (3 commits) (flutter/engine#9327) (flutter#34484)
  ace5d59 Fix rawTypes errors in Android embedding classes (flutter/engine#9326) (flutter#34481)
  bf0def6 Roll src/third_party/skia 4c4945a25248..87e885038893 (1 commits) (flutter/engine#9325) (flutter#34471)
  Roll engine f1d821d..6f5347c (13 commits) (flutter#34466)
  Allow "from" hero state to survive hero animation in a push transition (flutter#32842)
  Roll pub dependencies (flutter#33677)
  Skip flaky test on Windows (flutter#34464)
  Allow flaky tests to pass or fail and mark web tests as flaky (flutter#34456)
  Dont depend on web SDK unless running tests on chrome (flutter#34457)
  Fix semantics_tester (flutter#34368)
  Include raw value in Diagnostics json for basic types (flutter#34417)
  Refactor Gradle plugin (flutter#34353)
  Allow web tests to fail in cirrus config (flutter#34436)
  skip bottom_sheet (flutter#34430)
  Remove unused flag `--target-platform` from `flutter run` (flutter#34369)
  Extract DiagnosticsNode serializer from WidgetInspector (flutter#34012)
  ...
This was referenced Jul 8, 2019
Closed
Merged
@Piinks Piinks deleted the sliverFix branch July 9, 2019 17:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@goderbauer goderbauer goderbauer approved these changes

@chunhtai chunhtai Awaiting requested review from chunhtai

@dnfield dnfield Awaiting requested review from dnfield

Assignees

@Piinks Piinks

Labels
f: scrolling Viewports, list views, slivers, etc. 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.

SliverFillRemaining fill more than remaining space available
5 participants
@Piinks @dnfield @goderbauer @googlebot @chunhtai