-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
minor filter select enhancements #3933
Conversation
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.
Overall looks good. If you could also add 2/3 tests to verify this here https://github.com/apache/incubator-superset/blob/master/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx it'd be 👍
superset/config.py
Outdated
@@ -41,6 +41,8 @@ | |||
|
|||
ROW_LIMIT = 50000 | |||
VIZ_ROW_LIMIT = 10000 | |||
# max rows retrived by filter select auto complete |
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.
retrieved*
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.
done.
@@ -28,6 +28,7 @@ export default class FilterControl extends React.Component { | |||
})); | |||
this.state = { | |||
filters: initialFilters, | |||
xhr: null, |
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.
I would rename this to activeRequest
or something more descriptive
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.
done.
$.ajax({ | ||
// if there is an outstanding request to fetch values, cancel it. | ||
if (this.state.xhr) this.state.xhr.abort(); | ||
this.setState({ xhr: $.ajax({ |
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.
please format this section of code
this.setState({
xhr: $.ajax({
...
})
});
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.
done.
* `values_for_column` configurable row limit * `FilterControl` cancels active ajax request if any
* `values_for_column` configurable row limit * `FilterControl` cancels active ajax request if any
filter select
is hardcoded to 10K in the code, this PR makes the limit configurable.