Make DropdownMenu be able to scroll to the highlighted item when searching.
#129740
+49
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
QuncCccccc
force-pushed
the
ensure_visible_dropdown_menu
branch
from
c0f5989 to
5885da8
Compare
HansMuller
reviewed
Jun 29, 2023
Contributor
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
Looks good, just some questions about the details.
|
|
||
| if (widget.enableSearch) { | ||
| currentHighlight = search(filteredEntries, _textEditingController); | ||
| scrollToHighlight(); |
Contributor
There was a problem hiding this comment.
If currentHighlight is null should we still call scrollToHighlight()?
Contributor
Author
There was a problem hiding this comment.
No, I should add a null check here. Thanks for catching this! Just updated.
| void didUpdateWidget(DropdownMenu<T> oldWidget) { | ||
| super.didUpdateWidget(oldWidget); | ||
| if (oldWidget.dropdownMenuEntries != widget.dropdownMenuEntries) { | ||
| filteredEntries = widget.dropdownMenuEntries; |
Contributor
There was a problem hiding this comment.
Should currentHighlight be set to null here?
Contributor
Author
There was a problem hiding this comment.
Ah yes, and I think I need to add a if statement to check oldWidget.enableSearch != widget.enableSearch so that if widget.enableSearch is updated to false, currentHighlight should also be updated. Just updated.
if (oldWidget.enableSearch != widget.enableSearch) {
if (!widget.enableSearch) {
currentHighlight = null;
}
}
|
|
||
| void scrollToHighlight() { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| if (currentHighlight != null && buttonItemKeys[currentHighlight!].currentContext != null) { |
Contributor
There was a problem hiding this comment.
Is it possible to get here if currentHighlight is null? If so, we should test that case.
It would be nice to write this with only one lookup of the current highlight's context. Like
void scrollToHighlight() {
WidgetsBinding.instance.addPostFrameCallback((_) {
if (currentHighlight == null) {
return;
}
final BuildContext? highlightContext = buttonItemKeys[currentHighlight!].currentContext;
if (highlightContext != null) {
Scrollable.ensureVisible(highlightContext);
);
}
}
Contributor
Author
There was a problem hiding this comment.
I think the currentHighlight will not be null here because it is checked before calling scrollToHighlight(), so just removed it in this method.
QuncCccccc
force-pushed
the
ensure_visible_dropdown_menu
branch
from
600e7ba to
d199498
Compare
QuncCccccc
force-pushed
the
ensure_visible_dropdown_menu
branch
from
d199498 to
58fae51
Compare
github-actions
bot
added
framework
QuncCccccc
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 1, 2023
… when searching. (flutter/flutter#129740)
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Jul 1, 2023
flutter/flutter@ff838bc...aa5f4a2 2023-07-01 [email protected] Enable no_wildcard_variable_uses lint (flutter/flutter#129858) 2023-06-30 [email protected] mark packages-autoroller bringup again (flutter/flutter#129859) 2023-06-30 [email protected] Update `SwitchTheme` tests for M2/M3 (flutter/flutter#129811) 2023-06-30 [email protected] Change pub roller bot to push to flutter-pub-roller-bot/flutter.git (flutter/flutter#129844) 2023-06-30 [email protected] Fix NetworkImage causing spurious warning in tests (flutter/flutter#129537) 2023-06-30 [email protected] Roll Flutter Engine from 54b573e9c4e5 to e6b8292705a8 (4 revisions) (flutter/flutter#129852) 2023-06-30 [email protected] Upgrade integration tests to use AGP 7.3/Gradle 7.4 (flutter/flutter#129642) 2023-06-30 [email protected] Roll Packages from d4752c4 to 53ed5a0 (5 revisions) (flutter/flutter#129837) 2023-06-30 [email protected] Updated correct tasks for test ownership fix (flutter/flutter#129812) 2023-06-30 [email protected] Updated some golden image tests for M2/M3 (flutter/flutter#129794) 2023-06-30 [email protected] Remove an unnecessary assert (flutter/flutter#129796) 2023-06-30 [email protected] Update `Radio` tests for M2/M3 (flutter/flutter#129814) 2023-06-30 [email protected] Update `Switch` tests for M2/M3 (flutter/flutter#129810) 2023-06-30 [email protected] Roll Flutter Engine from 099a70ebbc60 to 54b573e9c4e5 (1 revision) (flutter/flutter#129821) 2023-06-30 [email protected] Update `SwitchListTile` tests for M2/M3 (flutter/flutter#129809) 2023-06-30 [email protected] Fix `NavigationDrawer` selected item has wrong icon color (flutter/flutter#129625) 2023-06-30 [email protected] Update basic_test.dart for M3 compliance (flutter/flutter#129714) 2023-06-30 [email protected] Roll Flutter Engine from d33343430f18 to 099a70ebbc60 (7 revisions) (flutter/flutter#129818) 2023-06-30 [email protected] Roll Flutter Engine from 68cc1a7971d5 to d33343430f18 (2 revisions) (flutter/flutter#129801) 2023-06-30 [email protected] Revert no-response to fork. (flutter/flutter#129775) 2023-06-30 [email protected] Make `DropdownMenu` be able to scroll to the highlighted item when searching. (flutter/flutter#129740) 2023-06-29 [email protected] Roll Flutter Engine from cd9ce66db14a to 68cc1a7971d5 (10 revisions) (flutter/flutter#129799) 2023-06-29 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 4.1.0 to 4.2.0 (flutter/flutter#129797) 2023-06-29 [email protected] Roll Flutter Engine from eabb22900b44 to cd9ce66db14a (1 revision) (flutter/flutter#129756) 2023-06-29 [email protected] Remove `@NonNull` to avoid warning (flutter/flutter#129472) 2023-06-29 [email protected] Remove use of any (flutter/flutter#129793) 2023-06-29 [email protected] Prepare for utf8.encode() to return more precise Uint8List type (flutter/flutter#129769) 2023-06-29 [email protected] Deletes files that should be ignored (flutter/flutter#127984) 2023-06-29 [email protected] Fix flutter_plugins by rolling revision (flutter/flutter#129781) 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://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
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 16, 2023
… when searching. (flutter/flutter#129740)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 17, 2023
… when searching. (flutter/flutter#129740)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 17, 2023
… when searching. (flutter/flutter#129740)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 17, 2023
… when searching. (flutter/flutter#129740)
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #120349
This PR enables
DropdownMenuto automatically scroll to the first matching item whenenableSearchis true.video example
pr_video.mov
Pre-launch Checklist
///).