feat: add raw sliver app bar#182925
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new RawSliverAppBar widget to abstract the non-Material logic from SliverAppBar. The existing SliverAppBar is refactored to use this new raw widget, which simplifies its implementation. New comprehensive tests for RawSliverAppBar are also added. The changes improve modularity and separation of concerns. I've included one suggestion to improve performance by memoizing a builder function.
|
This spun out of this comment on another PR: #182702 (comment) |
There was a problem hiding this comment.
The approach LGTM as long as tests still pass, but I'll defer to @Piinks . Thanks for jumping on this refactor @rkishan516.
119c6a8 to
dcbd38e
Compare
Piinks
left a comment
There was a problem hiding this comment.
Hey @rkishan516 I really appreicate all of the work you have put in here, this is not trivial change. However, SliverAppBar is a widget we listed in #53059 as one we do not want to refactor to a Raw widget.
These are some of the reasons we listed there:
- Highly complex and material-specific
- SliverPersistentHeader, its base class is already in widgets, We added several atomic sliver header widgets last year that are meant to replace the SliverAppBar behemoth through composition in the widgets layer.
- SliverResizingHeader: Pins its child and resizes it from its max extent to its min extent when scrolled
- SliverFloatingHeader: Hides the child when the user scrolls backwards and shows its child when the user scrolls forward
- PinnedHeaderSliver: Keeps its child pinned to the top of the scroll view.
The other listed slivers are intended to provide that more basic support for building dynamic headers that can be composed for various behaviors.
If we were to add more slivers to the widgets layer that handle header-like presentations, they should be specifically scoped to features (perhaps dynamic sizing) that are not already supported.
| BuildContext context, { | ||
| required double toolbarOpacity, | ||
| required double bottomOpacity, | ||
| required bool isScrolledUnder, |
There was a problem hiding this comment.
This is a material convention.
| final double minExtent; | ||
|
|
||
| /// Maximum height of the app bar when fully expanded. | ||
| final double maxExtent; |
There was a problem hiding this comment.
The fact that SliverAppBar cannot be dynamically sized is a long time pain point and not something we would want to continue in a new basic widget. (#18345)
|
Makes sense. Closing it then. |
This PR adds RawSliverAppBar widget and uses it for SliverAppBar which is material version.
Part of: #182702
Pre-launch Checklist
///).