-
-
Notifications
You must be signed in to change notification settings - Fork 532
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
DropdownList: Filter text input in the menu #5915
base: next-2.0
Are you sure you want to change the base?
Conversation
if ( TextField == null ) | ||
return; | ||
|
||
filteredData = Data.Where( x => TextField.Invoke( x ).Contains( FilterText, StringComparison.OrdinalIgnoreCase ) ); |
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.
Do we need to store filtered data? Or can we just return it as
return Data.Where( x => TextField.Invoke( x ).Contains( FilterText, StringComparison.OrdinalIgnoreCase ) );
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.
Yeah, in this the filteredData
field isn't needed. That implementation can go without it.
I took it form the Autocomplete
, where it is an IList
and the data aren't enumerated on every get. But made it IEnumberable
, because the Data are also IEnumerable
.
Well - it is a question now. Should the DropdownList.FilteredData
also be an IList
instead of IEnumerable
?
- IEnumerable will do the filtering on every enumeration. Possibly on every re-render. This could have significant perf implications.
- If converted to List, the filtering will be done only on text change, but it needs to fit into memory.... This is also a concern for current implementation of
Autocomplete
...
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.
Yeah, performance can be a problem. Dropdown, from a UX perspective is meant to be used only for small menus. With this new search features we are essentially breaking that "behavior" since we are allowing for very large menus. I guess that is fine if our users would expect that. But now we have a dilemma. Should we optimize or leave it as it is.
I think we can leave it as is, and optimize with caching later if it becomes a problem.
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, the current state is final. filteredData
is "needed" to avoid repeating all the checks unnecessarily, even when the filter isn’t dirty.
Another option is to keep everything (Data
and FilteredData
) as List -> that would signal "small" dataset and avoid unnecessary filtering on every get
...
Description
Closes #5666
Adds a filter to the
DropdownList
, bringing its functionality closer to whatAutocomplete
offers.The current solution has limited customization options for both the filter functionality and its appearance. It might be worth considering merging some features with
Autocomplete
—for example, allowing customization of the filter type.I’ll update the documentation and release notes once the core functionality is agreed upon.