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
feat(native-filters): Hide non-numeric columns in numeric range filter #15385
feat(native-filters): Hide non-numeric columns in numeric range filter #15385
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.
A few comments
@@ -103,6 +128,7 @@ export function ColumnSelect({ | |||
onChange={onChange} | |||
options={options} | |||
placeholder={t('Select a column')} | |||
notFoundContent={t('No columns found')} |
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.
Should we make this message display the currentFilterType
when set, just in case? Something like "No numerical columns found" if filtering for numerical columns etc.
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.
Replaced with No compatible columns found
, adding currentFilterType
would require an ugly name mapping
|
||
const FILTERS_FIELD_NAME = 'filters'; | ||
|
||
export const FILTER_GROUPS = { | ||
TIME: ['filter_time', 'filter_timegrain', 'filter_timecolumn'], |
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.
nit: would TEMPORAL
be more accurate? Also, would it be a good idea to list the supported datatypes instead?
{
filter_time: [GenericDataType.TEMPORAL],
filter_timegrain: [GenericDataType.TEMPORAL],
filter_timecolumn: [GenericDataType.TEMPORAL],
filter_select: [GenericDataType.STRING, GenericDataType.NUMERIC, GenericDataType.TEMPORAL],
filter_range: [GenericDataType.NUMERIC],
}
or similar.
edit: long-term this info should be moved into the plugin metadata
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.
Fantastic suggestion, thank you!
Codecov Report
@@ Coverage Diff @@
## master #15385 +/- ##
==========================================
- Coverage 77.26% 77.10% -0.17%
==========================================
Files 975 975
Lines 50579 50610 +31
Branches 6204 6212 +8
==========================================
- Hits 39081 39021 -60
- Misses 11291 11382 +91
Partials 207 207
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
LGTM. Only one non-blocking suggestion.
I really liked the improvements 👏🏼
.filter(filterValues) | ||
.map((col: Column) => col.column_name) | ||
.sort((a: string, b: string) => a.localeCompare(b)) | ||
.map((column: any) => ({ label: column, value: column })), |
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.
.map((column: any) => ({ label: column, value: column })), | |
.map((column: string) => ({ label: column, value: column })), |
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.
LGTM!
apache#15385) * feat(native-filters): Hide non-numeric columns in numeric range filter * Return true if type_generic undefined * Code review comments * Replace any with string * fix tests * add missing columns to select Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
apache#15385) * feat(native-filters): Hide non-numeric columns in numeric range filter * Return true if type_generic undefined * Code review comments * Replace any with string * fix tests * add missing columns to select Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
apache#15385) * feat(native-filters): Hide non-numeric columns in numeric range filter * Return true if type_generic undefined * Code review comments * Replace any with string * fix tests * add missing columns to select Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
apache#15385) * feat(native-filters): Hide non-numeric columns in numeric range filter * Return true if type_generic undefined * Code review comments * Replace any with string * fix tests * add missing columns to select Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
SUMMARY
When user selects "Numerical range" native filter, only numeric columns should be available to choose. If there are no numeric columns in the dataset, show "No columns found" message in select column dropdown. When user has selected a non-numeric column and then changes filter type to numerical range, the column field should be reset.
We use
type_generic
field to check column's type. If a column doesn't havetype_generic
field, we assume that it's numeric as a failsafe.Thank you @dpgaspar and @villebro for providing the backend code!
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2021-06-25.at.12.21.23.mov
TESTING INSTRUCTIONS
DASHBOARD_NATIVE_FILTERS
feature flagmessage_channels
, have columns withtype_generic
field unset. In those cases, we display them as a failsafe)ADDITIONAL INFORMATION