-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Sliver Main Axis Group #126596
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
Sliver Main Axis Group #126596
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 (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Piinks
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.
@thkim1011 is this ready for a review?
Piinks
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.
Can you add some sample code that illustrates the pinned case?
| /// A sliver that places multiple sliver children in a linear array along the | ||
| /// main axis. | ||
| /// | ||
| /// Typically, the slivers will be laid out one at a time. Slivers that have been |
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.
In what case would this order of layout not occur?
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.
I've updated the documentation to remove this line.
| /// main axis. | ||
| /// | ||
| /// Typically, the slivers will be laid out one at a time. Slivers that have been | ||
| /// scrolled past partially or entirely will be provided a nonzero scrollOffset that |
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 this referring to the pinned header case? I am not sure I follow. Also, if scrollOffset is a reference, can you add a [breadcrumb]?
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.
I've updated the documentation.
| /// the total layout extent of the sliver by painting slivers that are "out of | ||
| /// bounds" with a negative [SliverPhysicalParentData.paintOffset]. |
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.
I think I see now. It is bit difficult to discern which sliver you are referring to, the group as a whole or its children, can you clarify?
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.
I clarified this point.
|
|
||
| @override | ||
| void paint(PaintingContext context, Offset offset) { | ||
| RenderSliver? child = lastChild; |
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.
Can you share why this is the painting order?
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.
I think visitChildForSemantics expects the children to be visited in paint order, so that may need to be overridden to work with this reverse paint order.
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.
Gotcha. This is the paint order because this is how Viewport paints its sliver children. I think it's necessary for SliverPersistentHeader to be painted on top of the widgets that come after 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.
Are you going to implement visitChildrenForSemantics?
| /// the main axis, one after another. | ||
| /// | ||
| /// For pinned sliver children, the behavior is that the pinned sliver should | ||
| /// scroll up after all of the main content has been scrolled through. |
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.
Can you add a See also for SliverCrossAxisGroup? And then add one for the cross axis group that refers back here?
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.
Done!
| /// A sliver that places multiple sliver children in a linear array along | ||
| /// the main axis, one after another. | ||
| /// | ||
| /// For pinned sliver children, the behavior is that the pinned sliver should |
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.
Can you reference the API you are referring to here? Folks may not know what a pinned sliver child is in this context. Some sample code of this exact case would be really great actually. :)
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.
Done!
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.
I am not sure what this paragraph is saying, can you elaborate?
|
@Piinks I was wondering how to achieve the following. There are four nested listviews inside another external/parent listview. All vertical. When user starts scrolling the first nested listview and it reaches the end of scroll, now when trying to scroll the same nested listview will make the outer listview start scrolling. Is there a inbuilt solution for this in Flutter? Please let me know. |
Piinks
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
41a6d2d to
adb1f06
Compare
|
auto label is removed for flutter/flutter, pr: 126596, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
flutter/flutter@6e254a3...da127f1 2023-06-09 [email protected] Updated material button theme tests for Material3 (flutter/flutter#128543) 2023-06-09 [email protected] Roll Flutter Engine from cb93477008d6 to 93afba901b3b (2 revisions) (flutter/flutter#128573) 2023-06-09 [email protected] Improve defaults generation with logging, stats, and token validation (flutter/flutter#128244) 2023-06-09 [email protected] [testing] Make the FLUTTER_STORAGE_BASE_URL warning non-fatal (flutter/flutter#128335) 2023-06-09 [email protected] [flutter_tools] [DAP] Don't try to restart/reload if app hasn't started yet (flutter/flutter#128267) 2023-06-09 [email protected] Roll Flutter Engine from 8f9e608d39ab to cb93477008d6 (3 revisions) (flutter/flutter#128568) 2023-06-09 [email protected] Replace `MaterialButton` from test classes (flutter/flutter#128466) 2023-06-09 [email protected] Fix `showBottomSheet` doesn't remove scrim when draggable sheet is dismissed (flutter/flutter#128455) 2023-06-09 [email protected] Manual roll Flutter Engine from a5f7d5d75ff2 to 8f9e608d39ab (31 revisions) (flutter/flutter#128554) 2023-06-09 [email protected] Revert "test owners: cyanglaz -> vashworth" (flutter/flutter#128462) 2023-06-09 [email protected] [Android] Bump integration tests using `compileSdkVersion` 31 to 33 (flutter/flutter#128072) 2023-06-09 [email protected] Remove single view assumption from MouseTracker, and unify its hit testing code flow (flutter/flutter#127060) 2023-06-09 [email protected] [flutter_tools] Precache after channel switch (flutter/flutter#118129) 2023-06-08 [email protected] Adding migration guide for Material 3 colors (flutter/flutter#128429) 2023-06-08 [email protected] Add `AppLifecycleListener`, with support for application exit handling (flutter/flutter#123274) 2023-06-08 [email protected] Sliver Main Axis Group (flutter/flutter#126596) 2023-06-08 [email protected] Reduce `_DoubleClampVisitor` false positives (flutter/flutter#128539) 2023-06-08 [email protected] Advise developers to use OverflowBar instead of ButtonBar (flutter/flutter#128437) 2023-06-08 [email protected] Reland "Migrate benchmarks to package:web" (flutter/flutter#128266) 2023-06-08 [email protected] Navigator.pop before PopupMenuItem onTap call (flutter/flutter#127446) 2023-06-08 [email protected] Fix navigation rail with long labels misplaced highlights (flutter/flutter#128324) 2023-06-08 [email protected] Update `chip.dart` to use set of `MaterialState` (flutter/flutter#128507) 2023-06-08 [email protected] Update flutter to dartdoc 6.3.0 and hide Icons implementation from doc pages (flutter/flutter#128442) 2023-06-08 [email protected] Disable blinking cursor when `EditableText.showCursor` is false (flutter/flutter#127562) 2023-06-08 [email protected] [floating_cursor_selection]add more comments on the tricky part (flutter/flutter#127227) 2023-06-08 [email protected] Move RenderObjectElement.updateChildren to Element (flutter/flutter#128458) 2023-06-08 [email protected] Fix PointerEventConverter doc (flutter/flutter#128452) 2023-06-08 [email protected] Roll Packages from a84b2c2 to e13b8c4 (9 revisions) (flutter/flutter#128508) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
I noticed issue #108289 regarding sticky headers was closed, since @Piinks mentioned that SliverMainAxisGroup resolved it. I just tested SliverMainAxisGroup using two pinned SliverPersistentHeaders in the main branch here using the following code and the sticky header feature still doesn't work: |
@Areopagitics probably you should wrap each header in different SliverMainAxisGroup in order to make it work |
|
@rMozes Thank you so much! I'm guessing, though, that this feature will take another month or so to land in the stable branch. Here is some sample code for Sticky Headers that others can test here for future reference: |
|
I've been testing |
This is correct. Regarding the behavior of
Please file an issue request if you would like to see this work differently. Comments on old PRs are typically not responded to. |
Fixes #145068 The original tests for SliverMainAxisGroup did not actually check where the child was painted (#126596). Followed the same pattern in RenderSliverMultiBoxAdaptor: https://github.com/flutter/flutter/blob/11c034f0371eb28576d41a0c218ccae6b38e7702/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart#L666
This widget implements the ability to place slivers one after another in a single ScrollView in a way that all child slivers are drawn within the bounds of the group itself (i.e. SliverPersistentHeaders aren't drawn outside of the scroll extent provided by all of the child slivers). The design document for SliverMainAxisGroup can be found here.
Fixes #33137.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.