You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/09 12:12:34 UTC
[GitHub] [superset] kgabryje opened a new pull request #13029: fix(explore): Enable selecting an option not included in suggestions
kgabryje opened a new pull request #13029:
URL: https://github.com/apache/superset/pull/13029
### 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 refactor `src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx` to 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 https://github.com/apache/superset/issues/13017#issuecomment-775607176.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: fixes https://github.com/apache/superset/issues/13017
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r573508780
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc merged pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
junlincc merged pull request #13029:
URL: https://github.com/apache/superset/pull/13029
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #13029:
URL: https://github.com/apache/superset/pull/13029#issuecomment-775894255
@junlincc I didn't attach a video "after", because I couldn't find a dataset with >10k rows in a column. I'd appreciate if did some tests 🙂
@villebro @ktmud @nikolagigic Can you do a review please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r575821616
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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 components 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
junlincc commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r574994855
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review comment:
is there a reason why we didn't identify limitations or raise concern before the SIP was voted in?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r573536654
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review comment:
Thanks for the clarification!
Things like this makes me wonder [again](https://github.com/apache/superset/pull/12649) 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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r573507256
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review comment:
You probably don't need this since AntD select [has a "tags" mode](https://ant.design/components/select/#components-select-demo-tags), which behave similar to a freeForm react-select (`CreatableSelect`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r574724061
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
junlincc commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r575922190
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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......
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r575026795
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review comment:
Not saying we should definitely revert. Just laying out some concerns here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r575821616
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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).
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r575023321
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r573536654
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review comment:
Ah, thanks for the clarification! I totally missed that.
Things like this makes me wonder [again](https://github.com/apache/superset/pull/12649) 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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
junlincc commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r574636326
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje edited a comment on pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
kgabryje edited a comment on pull request #13029:
URL: https://github.com/apache/superset/pull/13029#issuecomment-775894255
@junlincc I couldn't find a dataset with >10k rows in a column. I'd appreciate if you did some tests 🙂
@villebro @ktmud @nikolagigic Can you do a review please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r575821616
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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).
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r575025498
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13029:
URL: https://github.com/apache/superset/pull/13029#issuecomment-777680279
one follow up task - show truncated label dynamically on panel resizing, not priority.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #13029: fix(explore): Enable selecting an option not included in suggestions
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13029:
URL: https://github.com/apache/superset/pull/13029#discussion_r575821616
##########
File path: superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
##########
@@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
+ onSearch={val => this.setState({ currentSuggestionSearch: val })}
+ onSelect={this.clearSuggestionSearch}
+ onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}
+
+ {/* enable selecting an option not included in suggestions */}
+ {currentSuggestionSearch &&
+ !this.state.suggestions.some(
+ suggestion => suggestion === currentSuggestionSearch,
+ ) && (
+ <Select.Option value={currentSuggestionSearch}>
+ {currentSuggestionSearch}
+ </Select.Option>
+ )}
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org