villebro commented on a change in pull request #15454:
URL: https://github.com/apache/superset/pull/15454#discussion_r662008707
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx
##########
@@ -124,7 +131,8 @@ export function ColumnSelect({
return (
<Select
- value={value}
+ mode={mode}
+ value={mode === 'multiple' ? value || [] : value}
Review comment:
When testing it felt like this could make sense for the groupby filter
at least:
```js
allowClear={mode === 'multiple'}
```
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
##########
@@ -98,4 +98,4 @@ export const hasTemporalColumns = (
export const doesColumnMatchFilterType = (filterType: string, column: Column)
=>
!column.type_generic ||
!(filterType in FILTER_SUPPORTED_TYPES) ||
- FILTER_SUPPORTED_TYPES[filterType].includes(column.type_generic);
+ FILTER_SUPPORTED_TYPES[filterType]?.includes(column.type_generic);
Review comment:
nit: This change isn't necessary due to `!(filterType in
FILTER_SUPPORTED_TYPES)` above (but I don't mind having it there if the logic
changes later)
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
##########
@@ -352,8 +344,8 @@ const FiltersConfigForm = (
// @ts-ignore
const hasDataset = !!nativeFilterItems[formFilter?.filterType]?.value
?.datasourceCount;
- const hasColumn =
- hasDataset && !FILTERS_WITHOUT_COLUMN.includes(formFilter?.filterType);
+ const hasColumn = hasDataset;
Review comment:
Agreed, let's remove it if it's not needed anymore
##########
File path:
superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx
##########
@@ -68,7 +68,15 @@ export default function PluginFilterGroupBy(props:
PluginFilterGroupByProps) {
// so we can process it like this `JSON.stringify` or start to use `Immer`
}, [JSON.stringify(defaultValue), multiSelect]);
- const columns = data || [];
+ const columns = data
+ ? formData.groupby && formData.groupby[0] && formData.groupby[0].length
Review comment:
This is slightly heavy to read. Could we break this up a bit, something
like
```js
const groupby = ensureIsArray(formData.groupby);
const columns = data ? data.filter(dataItem =>
groupby.includes(dataItem.columnName) : [];
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
