PinnedHeaderSliver #143196
PinnedHeaderSliver #143196
Conversation
github-actions
bot
added
framework
HansMuller
force-pushed
the
pinned_header_sliver
branch
from
dd80128 to
3a034c6
Compare
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
This sliver is preferable to the general purpose SliverPersistentHeader - for its relatively narrow use case - because there's no need to create a [SliverPersistentHeaderDelegate] or to predict the header's size.
Why not create a constructor for SliverPersistentHeader that does not require the user to provide a delegate?
There was a problem hiding this comment.
Should this talk more about when to use one or the other?
Should folks expect this one to expand/stretch/collapse? Or have a shadow when scrolled under?
I wonder if users will request this sliver to ultimately adopt all the existing features of the pinned persistent header we already have.
I follow that this one can adapt to the size of the child, where the existing one does not and takes the size from the delegate. for posterity, why add a new sliver instead of expanding on the existing one?
There was a problem hiding this comment.
Just sharing more thoughts.. will folks expect us to add a FloatingHeaderSliver to use instead of a floating SliverPersistentHeader? FloatingPinnerHeaderSliver?
There was a problem hiding this comment.
I think it's better not to bundle all the possible behaviors into one sliver. Having one sliver per behavior keeps the APIs simple and scales nicely when new behaviors are added (in their own classes).
I did write a FloatingHeaderSliver but forgot to include it in this raft of slivers. I will add it.
There was a problem hiding this comment.
I could be wrong but we usually prefix with the Sliver moniker. :)
There was a problem hiding this comment.
You're right, that's a convention I should have followed. Will rename everything.
There was a problem hiding this comment.
This is vertical+not-reversed. There should also be a basic test for vertical+reverse, horizontal+not-reversed, and horizontal+reversed. These cases tend to come back to us in bug reports. :)
There was a problem hiding this comment.
I have added tests for the other cases.
I think having a separate class for the special cases is nicer. SliverPersistentHeader is already a bit of a Swiss Army Knife and adding more special purpose constructors would only make it more so. |
HansMuller
force-pushed
the
pinned_header_sliver
branch
from
4e761d5 to
e06f77a
Compare
HansMuller
force-pushed
the
pinned_header_sliver
branch
from
e06f77a to
4b81038
Compare
|
The customer tests failures are due to an existing |
That would also break every Flutter developer that uses the sliver_tools package, which is pretty popular. Can we use a different name instead? |
|
Sure: how about |
That feels pretty easily confused with other slivers, especially since this is meant to have a singular purpose, to be pinned. |
|
OK, SliverStartHeader? |
I think offline we agreed that, in light of DecoratedSliver, it would be ok to deviate from the typical |
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
HansMuller
force-pushed
the
pinned_header_sliver
branch
from
7a5da88 to
025ae47
Compare
|
Restored the original name, PinnedHeaderSliver to avoid collisions. |
HansMuller
force-pushed
the
pinned_header_sliver
branch
from
025ae47 to
96bda21
Compare
…l notification (#146654) The scroll notification events reported for a press-drag-release gesture within a scrollable on a touch screen device begin with a `ScrollStartNotification`, followed by a series of `ScrollUpdateNotifications`, and conclude with a `ScrollEndNotification`. This protocol can be used to defer work until an interactive scroll gesture ends. For example, you might defer updating a scrollable's contents via network requests until the scroll has ended, or you might want to automatically auto-scroll at that time. In the example that follows the CustomScrollView automatically scrolls so that the last partially visible fixed-height item is completely visible when the scroll gesture ends. Many iOS applications do this kind of thing. It only makes sense to auto-scroll when the user isn't actively dragging the scrollable around. It's easy enough to do this by reacting to a ScrollEndNotifcation by auto-scrolling to align the last fixed-height list item ([source code](https://gist.github.com/HansMuller/13e2a7adadc9afb3803ba7848b20c410)). https://github.com/flutter/flutter/assets/1377460/a6e6fc77-6742-4f98-81ba-446536535f73 Dragging the scrollbar thumb in a desktop application is a similar user gesture. Currently it's not possible to defer work or auto-scroll (or whatever) while the scrollable is actively being dragged via the scrollbar thumb because each scrollbar thumb motion is mapped to a scroll start - scroll update - scroll end series of notifications. On a desktop platform, the same code behaves quite differently when the scrollbar thumb is dragged. https://github.com/flutter/flutter/assets/1377460/2593d8a3-639c-407f-80c1-6e6f67fb8c5f The stream of scroll-end events triggers auto-scrolling every time the thumb moves. From the user's perspective this feels like a losing struggle. One can also detect the beginning and end of a touch-drag by listening to the value of a ScrollPosition's `isScrollingNotifier`. This approach suffers from a similar problem: during a scrollbar thumb-drag, the `isScrollingNotifier` value isn't updated at all. This PR refactors the RawScrollbar implementation to effectively use a ScrollDragController to manage scrolls caused by dragging the scrollbar's thumb. Doing so means that dragging the thumb will produce the same notifications as dragging the scrollable on a touch device. Now desktop applications can choose to respond to scrollbar thumb drags in the same that they respond to drag scrolling on a touch screen. With the changes included here, the desktop or web version of the app works as expected, whether you're listing to scroll notifications or the scroll position's `isScrollingNotifier`. https://github.com/flutter/flutter/assets/1377460/67435c40-a866-4735-a19b-e3d68eac8139 This PR also makes the second [ScrollPosition API doc example](https://api.flutter.dev/flutter/widgets/ScrollPosition-class.html#cupertino.ScrollPosition.2) work as expected when used with the DartPad that's part of API doc page. Desktop applications also see scroll start-update-end notifications due to the mouse wheel. There is no touch screen analog for the mouse wheel, so an application that wanted to enable this kind of auto-scrolling alignment would have to include a heuristic that dealt with the sequence of small scrolls triggered by the mouse wheel. Here's an example of that: [source code](https://gist.github.com/HansMuller/ce5c474a458f5f4bcc07b0d621843165). This version of the app does not auto-align in response to small changes, wether they're triggered by dragging the scrollbar thumb of the mouse wheel. Related sliver utility PRs: #143538, #143196, #143325.
HansMuller
force-pushed
the
pinned_header_sliver
branch
from
9c72773 to
919f3c5
Compare
HansMuller
added
the
autosubmit
flutter/flutter@fc19ecf...32081aa 2024-06-08 [email protected] [CupertinoActionSheet] Match colors to native (flutter/flutter#149568) 2024-06-08 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 1cdbebee1901 to 45cf05f7a580 (10 revisions) (#149944)" (flutter/flutter#149960) 2024-06-08 [email protected] Roll Flutter Engine from 1cdbebee1901 to 45cf05f7a580 (10 revisions) (flutter/flutter#149944) 2024-06-07 [email protected] Refactor `getRootWidgetSummaryTree` tests in `widget_inspector_test.dart` (flutter/flutter#149930) 2024-06-07 [email protected] Remove zanderso from TESTOWNERS (flutter/flutter#149935) 2024-06-07 [email protected] PinnedHeaderSliver (flutter/flutter#143196) 2024-06-07 [email protected] Fix test case in "getRootWidgetSummaryTree" test (flutter/flutter#149923) 2024-06-07 [email protected] Revert "Identify and re-throw our dependency checking errors in `flutter.groovy` (#149609)" (flutter/flutter#149918) 2024-06-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix `SegmentedButton` clipping when drawing segments (#149739)" (flutter/flutter#149927) 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],[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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
A sliver that remains “pinned” to the top of the scroll view. Subsequent slivers scroll behind it. Typically the sliver is created as the first item in the list however it can be inserted anywhere and it will always stop at the top of the scroll view. When the scrolling axis is vertical, the PinnedHeaderSliver’s height is defined by its widget child. Multiple PinnedHeaderSlivers will layout one after the other, once they've scrolled to the top.
This sliver is preferable to the general purpose SliverPersistentHeader - for its relatively narrow use case - because there's no need to create a [SliverPersistentHeaderDelegate] or to predict the header's size.
Here's a working demo in DartPad.
Screen.Recording.2024-02-08.at.5.04.27.PM.mov
Related sliver utility PRs: #143538, #143325, #127340.