-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
[explore] fix autocomplete on verbose names #5204
Conversation
@@ -224,8 +224,13 @@ export default class MetricsControl extends React.PureComponent { | |||
(option.column_name.toLowerCase().indexOf(valueAfterAggregate.toLowerCase()) >= 0); | |||
} | |||
return option.optionName && | |||
(!option.metric_name || !this.isAutoGeneratedMetric(option)) && |
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.
@GabeLoins I'm trying to understand !option.metric_name
why filtering this out?
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 the metrics control you can select saved_metrics, columns, or aggregates from the dropdown. That's why I use optionName
for comparison rather than metric name. In that check I'm saying if the option is not a saved metric, or if it is a saved metric but not an autogenerated saved metric
@mistercrunch as of this PR: we only filter simple metrics with names that are formatted as So simple saved metrics with fancy names should still be displayed. |
Currently when searching for metrics or groupbys, the autocomplete search functionality only matches based on the metric_name, though in some cases the verbose_name is displayed and disregarded for search purposes. Also another issue is that not all pre-defined metrics show up in the drop down which is confusing. Users may have simple metrics for which they setup a nice verbose name and/or description and expect to see those in the dropdown. This PR addresses it for metric and column-related dropdowns.
Got it, let's get the ones with a |
Codecov Report
@@ Coverage Diff @@
## master #5204 +/- ##
==========================================
+ Coverage 63.39% 63.45% +0.05%
==========================================
Files 261 261
Lines 19838 19842 +4
Branches 1995 1998 +3
==========================================
+ Hits 12577 12591 +14
+ Misses 7252 7242 -10
Partials 9 9
Continue to review full report at Codecov.
|
@@ -224,7 +224,7 @@ describe('MetricsControl', () => { | |||
|
|||
expect(!!wrapper.instance().selectFilterOption( | |||
{ type: 'VARCHAR(255)', column_name: 'source', optionName: '_col_source' }, | |||
'Sou', | |||
'sou', |
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.
FYI react-select always passes lower case strings so I removed the toLowerCase call.
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.
TIL!
@GabeLoins please review when you find a moment |
@@ -103,6 +103,10 @@ const groupByControl = { | |||
optionRenderer: c => <ColumnOption column={c} showType />, | |||
valueRenderer: c => <ColumnOption column={c} />, | |||
valueKey: 'column_name', | |||
filterOption: (opt, text) => ( |
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.
why does this only apply to the group by control? shouldn't we do the same for metric control?
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.
MetricsControl
has it's own control that internally defines filterOption
in it's own way. I alter MetricsControl.selectFilterOption
in this very PR to address this.
This reverts commit d5ebc43.
* [explore] fix autocomplete on verbose names Currently when searching for metrics or groupbys, the autocomplete search functionality only matches based on the metric_name, though in some cases the verbose_name is displayed and disregarded for search purposes. Also another issue is that not all pre-defined metrics show up in the drop down which is confusing. Users may have simple metrics for which they setup a nice verbose name and/or description and expect to see those in the dropdown. This PR addresses it for metric and column-related dropdowns. * Add unit test
…5219) * Revert "[explore] fix autocomplete on verbose names (apache#5204)" This reverts commit d5ebc43. * Revert "[webpack] setup lazy loading for all visualizations (apache#4727)" This reverts commit de0aaf4.
* [explore] fix autocomplete on verbose names Currently when searching for metrics or groupbys, the autocomplete search functionality only matches based on the metric_name, though in some cases the verbose_name is displayed and disregarded for search purposes. Also another issue is that not all pre-defined metrics show up in the drop down which is confusing. Users may have simple metrics for which they setup a nice verbose name and/or description and expect to see those in the dropdown. This PR addresses it for metric and column-related dropdowns. * Add unit test
…5219) * Revert "[explore] fix autocomplete on verbose names (apache#5204)" This reverts commit d5ebc43. * Revert "[webpack] setup lazy loading for all visualizations (apache#4727)" This reverts commit de0aaf4.
Currently when searching for metrics or groupbys, the autocomplete
search functionality only matches based on the metric_name, though in
some cases the verbose_name is displayed and disregarded for search
purposes.
Also another issue is that not all pre-defined metrics show up in the
drop down which is confusing. Users may have simple metrics for which
they setup a nice verbose name and/or description and expect to see
those in the dropdown.
This PR addresses it for metric and column-related dropdowns.
@GabeLoins @michellethomas