-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
Implement CupertinoSearchTextField opacity fade on scroll #155025
Implement CupertinoSearchTextField opacity fade on scroll #155025
Conversation
Works only on mobile platforms... Maybe something to do with |
void didChangeDependencies() { | ||
super.didChangeDependencies(); | ||
_ancestorScrollController?.removeListener(_onScroll); | ||
_ancestorScrollController = Scrollable.maybeOf(context)?.widget.controller; |
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.
This is probably why it is only working on mobile.
ScrollableState.widget.controller will only have a controller if the user has provided one, or the ancestor ScrollView decided to assign the PrimaryScrollController.
See the _effectiveScrollController in ScrollableState, which accounts for this. It may need to be made public for this feature, although, you could use ScrollableState.position and listen to that alternatively. :)
} | ||
|
||
void _onScroll() { | ||
if (_ancestorScrollPosition == null) { |
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.
You should check if the pixels
property can be used, with hasPixels
. The scroll position might not be attached.
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.
Overall, it looks good. I have some questions about behavior though.
For the partial scroll, so a scroll stops mid fade, can we put in snapping for this widget to start? It might be odd if it's partially shrunk and has faded text if the user tries to interact and type into it while it's partially shrunk.
setState(() { _fadeExtent = _calculateScrollOpacity(currentHeight, _maxHeight); }); | ||
} | ||
|
||
static double _calculateScrollOpacity(double currentHeight, double maxHeight) { |
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.
Does this assume that the search field is at the top of the scrollable content? What happens if it isn't?
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.
this is only useful when the search field is resizing (i.e it's height is reducing from its initial maximum height). The only reason it's called _calculateScrollOpacity
is because this resizing in this case must happen due to a scroll (because of the if-block on line 425). So its scroll position isn't really relevant and is used only to check that this resizing effect is happening in the midst of a scroll.
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.
TLDR we don't care about where the search field is in the scroll view, only that it is in a scroll view and resizing.
} | ||
|
||
static double _calculateScrollOpacity(double currentHeight, double maxHeight) { | ||
final double thresholdHeight = maxHeight * _kMinHeightBeforeTotalTransparency; |
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.
Question: should this only take height into account, or should padding be a factor? It might be fine to not get too granular in this to start though.
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.
It's a valid concern. Testing locally with different paddings, I don't think it will be a problem (for now) though.
An additional benefit of making it dependent on the height alone is that it guarantees that by the time the search field is small enough (that the prefix/suffix icons overflow), the contents have faded out completely.
I'm wondering if the snapping effect is more of a nav bar thing rather than a widget specific thing. Because in the settings app for instance, the padding/margin around the search bar also snaps along with the search bar itself. If we implement the snapping on the widget level, how would that play with the snapping on the nav bar level? |
@@ -398,13 +421,35 @@ class _CupertinoSearchTextFieldState extends State<CupertinoSearchTextField> | |||
} | |||
} | |||
|
|||
void _onScroll() { | |||
if (_ancestorScrollPosition == null || !_ancestorScrollPosition!.hasPixels) { |
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.
Today we talked about setting _maxHeight
here when _onScroll
is called for the first time by looking up the render object as you are below. 👍
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 that case, we may want to use the ScrollNotificationObserver. It would be more consistent with how the AppBar scrolledUnder behavior works, and we could get more information about what kind of scrolling is occurring.
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.
Code LGTM, but I'm not very informed on scrolling.
For the snapping on partial scroll: it makes an amount of sense to have that in a separate PR. Maybe with the drawer/ bottom work? But I'm just wondering if anything about this PR may get in the way of that. Is there a rough idea of how that could be implemented?
I don't think this PR will get in the way of that. For the snapping/autoscrolling, I think we can grab the ScrollController of the parent CustomScrollView, and animate the scroll by the necessary offset when we receive a ScrollEndNotification (and the title/bottom is partially scrolled under the edge of the viewport). This would probably be in its own PR. |
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 to me too! 🎉
Before:
364571986-9d3c59a3-5fda-47b2-993b-cc05b9eba596.1.mov
After:
Screen.Recording.2024-09-11.at.9.37.41.AM.mov
Native iOS:
364571955-39372e3d-e59a-43c7-a94b-b71038b1ee6e.mov
Fixes #154648
Needed for #18103
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.