Table multiselect filters #823
Conversation
Codecov Report
@@ Coverage Diff @@
## main #823 +/- ##
==========================================
- Coverage 85.44% 85.42% -0.02%
==========================================
Files 792 793 +1
Lines 16222 16230 +8
Branches 1930 1930
==========================================
+ Hits 13861 13865 +4
- Misses 2329 2333 +4
Partials 32 32
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| export const toInFilter = (tableFilters: TableFilter[]): TableFilter => | ||
| tableFilters.reduce((previousValue, currentValue) => ({ | ||
| field: previousValue.field, |
There was a problem hiding this comment.
what if the fields in the tableFilter are different?. In this reduce, we are just considering the last field as field for the final In-filter.
There was a problem hiding this comment.
Yeah, this is my issue with this too.
We could validate here or I can break up the args to ensure we only are passed one field and operator, but I felt it was overkill and traded a little bit of confusion (and a potential for a bug if used incorrectly) for ease of use.
I can add some validation easy enough, so I will. I can revisit other options if we feel strongly.
|
|
||
| interface TableControlOptionBase<T> { | ||
| value?: T; | ||
| export interface SelectControl { |
There was a problem hiding this comment.
This should be prefixed with 'Table'
| options: SelectOption<TableControlOption>[]; | ||
| export const enum TableControlOptionType { | ||
| Filter = 'filter', | ||
| Property = 'property', |
There was a problem hiding this comment.
What is a property control option? any visual example?
There was a problem hiding this comment.
Please see this comment below.
| export interface TableFilterControlOption<T = unknown> extends TableControlOptionBase<T> { | ||
| type: TableControlOptionType.Filter; | ||
| metaValue: TableFilter; | ||
| export type TableSelectControlOption = TableFilterControlOption; |
There was a problem hiding this comment.
do we really need so many different types? Like below where you specify metaValue type
export interface TableControlOption<T> {
type: TableControlOptionType;
label: string;
metaValue: T;
}
}
There was a problem hiding this comment.
I didn't add them just for fun! 😂
But valid question. I explain a little more in this comment.
| export interface TableUnsetControlOption { | ||
| type: TableControlOptionType.Unset; | ||
| label: string; | ||
| metaValue: string; |
There was a problem hiding this comment.
what is meta value supposed to do? How is it different than 'value' ?
There was a problem hiding this comment.
Please see this comment below.
| private isValidCheckboxControlOptions(options: TableControlOption[]): options is TableCheckboxOptions { | ||
| return ( | ||
| options.length === 2 && | ||
| options.every(option => hasOwnProperty(option, 'value') && typeof option.value === 'boolean') |
There was a problem hiding this comment.
I am yet to understand the inheritance in the table control options. value property seems to be defined at only one place. We can also use in here, i think
There was a problem hiding this comment.
Yeah, they are confusing. Fortunately it got a little bit cleaner with this PR.
Correct, that value is only for checkbox control now, since it has to hold a true or false while the select control doesn't. Checkbox controls are actually the oddball because they have to hold more ways of modifying a query.
Both select and checkbox controls have a metaValue. This is what contains the information about what to use in the query. It is most typically a Filter (see the metaValue for TableFilterControlOption), containing key, operator, and value. This is the metaValue that all select controls use now.
There are two other cases with query info, but they are now only used for checkbox controls. In addition to the Filter metaValue, there is an Unset and a Property metaValue (see the metaValue for TableUnsetControlOption and TablePropertyControlOption). These are also used to modify the query, but in a different way.
The Unset metaValue is used to hold a string that identifies which filter to remove from our filter list. The Property metaValue is a Dictionary used to add request properties to the query. Currently, our only example of this is includeInactive: true/false that we add to the request for APIs.
So, the reason that checkbox has a value and not the select dropdown is that checkbox has this additional user specified true/false in addition to the value that is buried in the metaValue for all controls.
Not sure if that explains it or makes it more confusing, but happy to expand on anything.
| assertUnreachable(changed.value); | ||
| if (changed.values.length === 0) { | ||
| // tslint:disable-next-line: no-void-expression | ||
| return this.selectFilterSubject.next(this.removeFilters(changed.select.options[0].metaValue.field)); |
There was a problem hiding this comment.
we don't need the return here right?
There was a problem hiding this comment.
Correct! Don't need to, but returning makes the code a little cleaner because we don't need an else block or a separate return; statement within the if block. Downside is I need the tslint disable. I could eliminate the return below, I suppose with the same results. It just looked odd.
There was a problem hiding this comment.
I honestly find this more confusing. If we can use a separate return i think that's more clear.
There was a problem hiding this comment.
This is the strangest thing to hold up a PR for.
There was a problem hiding this comment.
I'll change it in my local.
There was a problem hiding this comment.
It is a strong nit from my end
There was a problem hiding this comment.
I was really curious what this was about when seeing the notifications. At first I was on team jake, as I prefer unwrapping an else if possible, but then I realized that we were returning a void expression and anand won the day. No strong preference between if/else and a separate return though.
There was a problem hiding this comment.
I'm going to change it. Still silly to hold up a PR over this.
This comment has been minimized.
This comment has been minimized.
…ertrace-ui into table-multiselect-filters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Description
This change is to make all of the table control dropdowns multiselect instead of single select.
It also cleans up some messiness around the table control filter types. Using multi-select is actually easier to configure because it no-longer requires an
Unsetoption in the dropdown. This simplifies the API for theselectdropdowns and reduces its possible filter types to justTableControlOptionType.Filter. Now onlycheckboxtable controls useTableControlOptionType.UnsetandTableControlOptionType.Propertytypes.Screen.Recording.2021-05-05.at.1.52.04.PM.mov