[framework] SliverDecoration #107269

jonahwilliams · 2022-07-08T00:03:15Z

Avoid situations where users place scrollable content in a Column just so they can wrap it in a Container to add some kind of BoxDecoration. For example, I've seen stuff like:

CustomScrollView(
  slivers: [
    SliverToBoxAdaptor(
      child: Container(
         color: Colors.red,
         child: Column( ...),
      ),
    ),
  ]
)

Which totally defeats the purpose of scroll views

flutter-dashboard · 2022-07-08T19:35:24Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

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.

Changes reported for pull request #107269 at sha 7939874

flutter-dashboard · 2022-07-08T20:18:23Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #107269 at sha a5bb029

jonahwilliams · 2022-07-08T23:07:41Z

I don't think the golden tests I added are super useful, I'll update them a bit so they actually have some content

flutter-dashboard · 2022-07-11T22:45:24Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #107269 at sha a3822f6

jonahwilliams · 2022-07-11T23:00:25Z

Renamed to SliverDecoration to matching existing sliver widgets

Piinks

Love this! 😻

Can you add a reference in the docs for DecoratedBox and/or BoxDecoration to make this more discoverable?

Piinks · 2022-07-11T23:14:53Z

packages/flutter/lib/src/widgets/sliver_decoration.dart

+
+/// Paints a [Decoration] either before or after its child paints.
+class RenderSliverDecoration extends RenderProxySliver {
+  /// Creates a decorated box.


Nit

Suggested change

/// Creates a decorated box.

/// Creates a decorated sliver.

jonahwilliams · 2022-07-11T23:28:18Z

I added some breadcrumbs to the "see also" section of the BoxDecoration and DecoratedBox docs

Piinks

flutter-dashboard · 2022-07-12T00:21:13Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #107269 at sha ceef37a

Kavantix · 2022-07-12T19:58:16Z

A widget like this is super useful but this implementation should be reverted IMHO, sorry @jonahwilliams.
There are a couple things wrong here:

It doesnt work in horizontal scrollviews, (@Piinks I tested it to confirm)
It uses the paintExtent of the child which isn't always the correct value to use, take for instance this example when applying a rounde corner. The bottom two corners shouldn't be rounded yet.

Overlap and other advanced properties of slivers aren't accounted for.
Nit: The name is inconsistent with the box counterpart DecoratedBox, this should be called DecoratedSliver.

The correct behaviour should be the same as using a SliverStack from sliver_tools with a SliverPositioned.fill behind the second child sliver.

@Piinks @jonahwilliams let me know if you rather have a new ticket created for these issues, but I stand by my point that this current implementation should NOT reach stable or beta channels if possible. It will create more issues than it solves.

jonahwilliams · 2022-07-12T20:34:19Z

Well the only one I can immediately disagree with is 4, which is that I chose the name to be consistent with the other sliver widgets like SliverPadding and SliverList. 🐈

I agree we should fix those other issues, it would be helpful if you could file bugs for the specific ones you've noticed here. Thanks!

Kavantix · 2022-07-12T20:46:34Z

Well the only one I can immediately disagree with is 4, which is that I chose the name to be consistent with the other sliver widgets like SliverPadding and SliverList. 🐈

Well this name suggests it’s a counterpart to BoxDecoration

Kavantix · 2022-07-12T20:47:42Z

And probably there should actually be a SliverDecoration that has specific properties related to decorating slivers rather than boxes

jonahwilliams · 2022-07-12T21:04:18Z

Yes, but then it would be named differently from all of the other Sliver* widgets

Kavantix · 2022-07-12T21:14:34Z

You could technically name it SliverDecorated or SliverWithDecoration but DecoratedSliver is both nicer and more discoverable for someone that knows about DecoratedBox
This is also how I ended up with the name MultiSliver in sliver_tools, there just want a good name that started with Sliver, covered the load and wasnt unwieldy

jonahwilliams · 2022-07-12T21:38:46Z

I like SliverDecorated

jonahwilliams · 2022-07-12T21:39:15Z

but its a bit odd still IMO, we don't do past tense for the most part. I.e. its SliverPadding and not SliverPadded

Kavantix · 2022-07-12T21:51:44Z

I agree and wouldnt have even mentioned the name if it werent for BoxDecoration not being the widget but the configuration. And we’ll run into this same thing when inevitably adding a Sliver counterpart to ColoredBox.

Perhaps in light of this and the widget not being finished it would make more sense to not put it in the framework itself just yet?

This reverts commit 881b27e.

This is a second attempt to merge #107269. Currently I've fixed two of the issues: 1. Fixed horizontal scrollview by using a switch statement to consider vertical/horizontal case. 2. Fixed issue of `paintExtent` not being the right extent for painting. Rather using a `scrollExtent` for the main axis length of the decoration box and painting it offsetted by the `scrollOffset`. 3. If the sliver child has inifinite scrollExtent, then we only draw the decoration down to the bottom of the `cacheExtent`. The developer is expected to ensure that the border does not creep up above the cache area. This PR includes a test that checks that the correct rectangle is drawn at a certain scrollOffset for both the horizontal and vertical case which should be sufficient for checking that `SliverDecoration` works properly now. Fixes #107498.

[framework] decorated sliver

d112408

flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jul 8, 2022

add tests and use RenderProxySliver

91ecf02

jonahwilliams marked this pull request as ready for review July 8, 2022 18:16

++

ae01430

jonahwilliams requested review from Piinks and goderbauer July 8, 2022 18:25

Jonah Williams added 3 commits July 8, 2022 11:35

+_+

8dc4071

++

86647cf

++

7939874

flutter-dashboard bot added the will affect goldens Changes to golden files label Jul 8, 2022

remove test

a5bb029

update golden

a3822f6

jonahwilliams changed the title ~~[framework] DecoratedSliver~~ [framework] SliverDecoration Jul 11, 2022

rename to SliverDecoration and update goldens

a4daaf1

Piinks added the c: new feature Nothing broken; request for a new capability label Jul 11, 2022

Piinks reviewed Jul 11, 2022

View reviewed changes

add breadcrumbs in BoxDecoration and DecoratedBox docs

30637f7

Piinks approved these changes Jul 11, 2022

View reviewed changes

remember the name of the class

ceef37a

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 12, 2022

auto-submit bot merged commit 881b27e into flutter:master Jul 12, 2022

This was referenced Jul 12, 2022

Roll Flutter from d59923b2e260 to d092601debdd (35 revisions) flutter/plugins#6095

Merged

Roll Flutter from e17d28d7ab04 to d092601debdd (22 revisions) flutter/packages#2319

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jul 12, 2022

881b27e [framework] SliverDecoration (flutter/flutter#107269)

3fc6bc3

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 12, 2022

881b27e [framework] SliverDecoration (flutter/flutter#107269)

f18f0e1

Kavantix mentioned this pull request Jul 12, 2022

Implement SliverDecoration #107498

Closed

jonahwilliams pushed a commit that referenced this pull request Jul 13, 2022

Revert "[framework] SliverDecoration (#107269)"

375700e

This reverts commit 881b27e.

jonahwilliams mentioned this pull request Jul 13, 2022

Revert "[framework] SliverDecoration" #107561

Merged

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

[framework] SliverDecoration (flutter#107269)

5e313eb

thkim1011 mentioned this pull request May 30, 2023

DecoratedSliver #127823

Merged

[framework] SliverDecoration #107269

[framework] SliverDecoration #107269

Uh oh!

Conversation

jonahwilliams commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Jul 8, 2022

Uh oh!

flutter-dashboard bot commented Jul 8, 2022

Uh oh!

jonahwilliams commented Jul 8, 2022

Uh oh!

flutter-dashboard bot commented Jul 11, 2022

Uh oh!

jonahwilliams commented Jul 11, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jul 11, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jul 12, 2022

Uh oh!

Kavantix commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Jul 12, 2022

Uh oh!

Kavantix commented Jul 12, 2022

Uh oh!

Kavantix commented Jul 12, 2022

Uh oh!

jonahwilliams commented Jul 12, 2022

Uh oh!

Kavantix commented Jul 12, 2022

Uh oh!

jonahwilliams commented Jul 12, 2022

Uh oh!

jonahwilliams commented Jul 12, 2022

Uh oh!

Kavantix commented Jul 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Jul 8, 2022 •

edited

Loading

Kavantix commented Jul 12, 2022 •

edited

Loading