fix(explore): Enable selecting an option not included in suggestions#13029
Conversation
|
@junlincc I couldn't find a dataset with >10k rows in a column. I'd appreciate if you did some tests 🙂 |
junlincc
added
explore:filter
| <Select.Option value={currentSuggestionSearch}> | ||
| {currentSuggestionSearch} | ||
| </Select.Option> | ||
| )} |
There was a problem hiding this comment.
You probably don't need this since AntD select has a "tags" mode, which behave similar to a freeForm react-select (CreatableSelect).
There was a problem hiding this comment.
The problem is that tags allows to select multiple values, and operators like equals should work with a single value. That's why this "hack" was needed
There was a problem hiding this comment.
Ah, thanks for the clarification! I totally missed that.
Things like this makes me wonder again whether we should revert back to react-select... It seems AntD select simply doesn't have the feature completeness and flexibility we want. If this is about aesthetic improvement, we can always restyle react-select to look like AntD (or whatever new style we like).
There was a problem hiding this comment.
I see your concerns and they are all valid. Since we have gone so far in the migration already, it will be a major time waste reverting back. @kgabryje @villebro @zhaoyongjie let's allocate sometime to investigate and understand limitations of AntD, and have a long-term plan to address those concerns before diving deeper into renovating control panel.
There was a problem hiding this comment.
AFAIK, the only places we have replaced react-select are in the AdhocFilter and MetricsControl popovers. There are many more places that are currently using react-select. Please also consider future migration cost if we decide to all-in AntD select.
Note that react-select is still a very much actively maintained library, and have some powerful backers.
There was a problem hiding this comment.
As far as I can tell, from an end user perspective this UX is indistinguishable from something where this would be natively supported without the manually added Select.Option. As this solution is quite simple and not really a hack IMO (we are still using fully standard components here), I feel this is fine as-is.
There was a problem hiding this comment.
Do note though you'd have to do this for everywhere the current CreatableSelect is used, ultimately. So this hack might need to be generalized to a wrapper component if we are to keep the same UX.
It just doesn't feel right to me to introduce a hacky solution when there is a current solution that have worked perfectly fine.
There was a problem hiding this comment.
Not saying we should definitely revert. Just laying out some concerns here.
There was a problem hiding this comment.
I think adding a wrapper that hides the specifics of how the component actually implements the creatable option sounds like a good option. To my understanding this is essentially how the migration process has been carried out this far, i.e. importing components from src/common/components/, where we start out by just exporting the vanilla component, and later extending the component if needed. This way we can later much easier swap out the implementation of said component if a new better library comes along (or if the current implementation ends up being more problematic than we had envisioned). Granted, the abstracted component's props will end up looking fairly identical with their original implementation, but as we probably end up using only a small subset of the features, migrating to a new implementation will at least be slightly easier than not having any centralized abstraction.
Btw, I looked at what the current react-select implementation looks like in the Filter Box, and that flow seems quite hacky to the end user (see below). The select control that's used in the control panel isn't as bad, but it does go to show that the react-select experience in Superset has not been optimal from a UX perspective, either.
There was a problem hiding this comment.
Thank you @villebro, for further explanation. i honestly don't have enough knowledge to tell which library has less drawbacks or better extensibility. I am sure we can always tweak UI/UX and make improvement on either one. since we landed on AntD, let's stick with it and invest in one direction. also, thanks @ktmud for raising your concerns. Let's leave more time for discussion next time when it comes major changes like this. I read through the SIP and felt that we should have done more research back then......
|
one follow up task - show truncated label dynamically on panel resizing, not priority. |
junlincc
left a comment
junlincc
left a comment
There was a problem hiding this comment.
Approving as product signoff.thanks Kamil for the quick fix. :)
* master: (30 commits) refactor(native-filters): decouple params from filter config modal (first phase) (apache#13021) fix(native-filters): set currentValue null when empty (apache#13000) Custom superset_config.py + secret envs (apache#13096) Update http error code from 400 to 403 (apache#13061) feat(native-filters): add storybook entry for select filter (apache#13005) feat(native-filters): Time native filter (apache#12992) Force pod restart on config changes (apache#13056) feat(cross-filters): add cross filters (apache#12662) fix(explore): Enable selecting an option not included in suggestions (apache#13029) Improves RTL configuration (apache#13079) Added a note about the ! prefix for breaking changes to CONTRIBUTING.md (apache#13083) chore: lock down npm to v6 (apache#13069) fix: API tests, make them possible to run independently again (apache#13076) fix: add config to disable dataset ownership on the old api (apache#13051) add required * indicator to message content/notif method (apache#12931) fix: Retroactively add granularity param to charts (apache#12960) fix(ci): multiline regex in change detection (apache#13075) feat(style): hide dashboard header by url parameter (apache#12918) fix(explore): pie chart label bugs (apache#13052) fix: Disabled state button transition time (apache#13008) ...
junlincc
added
rush!
junlincc
removed
#bug:blocking!
mistercrunch
added
🏷️ bot
5 participants
SUMMARY
Filter suggestions endpoint has a 10k limit of returned results. If there are more than 10k possible values and the searched value is at the end of the list, it will not be returned and the user won't be able to select it when using a single value operator. This PR implements a "hack", which creates an additional option based on current search input, thus allowing to select it.
I consider this a temporary solution. Optimally, we should make the
/superset/filter/endpoint accept a search query param and use it to filter the results. Then we can refactorsrc/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsxto send a request for suggestions when a user types a query (with some debounce for optimization).This PR also fixes the issue with warning tooltip mentioned in #13017 (comment).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: see #13017
After:
TEST PLAN
ADDITIONAL INFORMATION