Repository navigation
Conversation
flutter-dashboard
Bot
added
framework
|
cc/ @gspencergoog I think I found a cleaner way to implement this. Still needs tests if this is the way to go, but WDYT? |
| /// for default ScrollActions to take place outside of th context fo the | ||
| /// current focus. | ||
| /// {@endtemplate} | ||
| final bool primaryScrollShortcut; |
There was a problem hiding this comment.
It think this needs a better name, I found it hard to know what it was before I read this documentation block.
Some ideas:
isDefaultPrimaryScrollableisFallbackScrollableisPrimaryScrollableisDefaultScrollable- Or something else...these don't feel perfect either.
I feel like it doesn't need the "Shortcut" as part of the name, since you could invoke the ScrollAction without using a shortcut if you wanted to. It should also probably begin with "is", since it's a bool.
There was a problem hiding this comment.
I see what you mean, I'll experiment with some new names. Thanks for the feedback! :)
| this.keyboardDismissBehavior = ScrollViewKeyboardDismissBehavior.manual, | ||
| this.restorationId, | ||
| this.clipBehavior = Clip.hardEdge, | ||
| this.isDefaultScrollAction = false, |
There was a problem hiding this comment.
Semantically, how is this different from primary (line 84)?
There was a problem hiding this comment.
Oh that is probably a much better flag to use rather than introducing a new one. Thanks!
| child: Overlay( | ||
| key: _overlayKey, | ||
| initialEntries: overlay == null ? _allRouteOverlayEntries.toList(growable: false) : const <OverlayEntry>[], | ||
| child: DefaultScrollAction( |
There was a problem hiding this comment.
The navigator is an odd place to add scrolling behavior.
| // Similar to notificationContext, _ScrollableScope is placed above the | ||
| // widget using it: RawGestureDetector, which allows us to access this | ||
| // ScrollableState | ||
| defaultScrollAction?._defaultScrollKey = _gestureDetectorKey; |
There was a problem hiding this comment.
What if we have two scrollables with isDefaultScrollAction set to true?
There was a problem hiding this comment.
In some situations that would probably be illegal. In others (e.g. if they are in separate routes) it would presumably be ok?
| child: Overlay( | ||
| key: _overlayKey, | ||
| initialEntries: overlay == null ? _allRouteOverlayEntries.toList(growable: false) : const <OverlayEntry>[], | ||
| child: DefaultScrollAction( |
There was a problem hiding this comment.
Also, wouldn't route A and B both have separate defaultScrollables? When route B is popped, how does Route A become the default again (I don't think there's any guarantee that the Scrollable's didChangeDependencies of Route A will run to re-register when Route B is popped.
|
Closing for now in favor of discussing an alternative solution in #69795 |
3 participants
Description
This introduces a new widget that will enable fallback/default keyboard scrolling for
ScrollActions. TheDefaultScrollActionmanages a givenScrollablecontext so if one is not found during the course of aScrollAction, the fallbackScrollablewill receive it.Earlier ideas for this involved the user passing a key to the
DefaultScrollAction, but this felt kludgy and bug prone, especially since the given key would need to be below theScrollablefor look-up. I've addedScrollable.isDefaultScrollActionto all associated classes instead to see if this is a better user experience. The boolean flag will indicate if theScrollableshould connect with theDefaultScrollAction, and can provide the right key for accessing the state of theScrollable. This also felt less exposed as we can leave most of the implementation private.Design Document
Related Issues
#43694
Tests
WIP
Checklist
Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.