[GitHub] [superset] villebro commented on a change in pull request #12199: feat(native-filters): Add defaultValue for Native filters modal

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #12199:
URL: https://github.com/apache/superset/pull/12199#discussion_r567894075



##########
File path: superset-frontend/src/dashboard/actions/nativeFilters.ts
##########
@@ -117,15 +118,18 @@ export function setFilterState(
  * Sets the selected option(s) for a given filter
  * @param filterId the id of the native filter
  * @param extraFormData the selection translated into extra form data
+ * @param currentState
  */
 export function setExtraFormData(
   filterId: string,
   extraFormData: ExtraFormData,
+  currentState: object,

Review comment:
       Same here?

##########
File path: superset-frontend/src/dashboard/actions/nativeFilters.ts
##########
@@ -99,6 +99,7 @@ export interface SetExtraFormData {
   type: typeof SET_EXTRA_FORM_DATA;
   filterId: string;
   extraFormData: ExtraFormData;
+  currentState: object;

Review comment:
       Should this be of type `CurrentFilterState`?

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar.tsx
##########
@@ -201,7 +202,11 @@ interface FilterProps {
   filter: Filter;
   icon?: React.ReactElement;
   directPathToChild?: string[];
-  onExtraFormDataChange: (filter: Filter, extraFormData: ExtraFormData) => void;
+  onExtraFormDataChange: (
+    filter: Filter,
+    extraFormData: ExtraFormData,
+    currentValue: any,

Review comment:
       I believe this was called `currentState` and was of type `CurrentFilterState`. Let's use same naming+type everywhere.

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/types.ts
##########
@@ -29,12 +29,13 @@ export type AntCallback = (value1?: any, value2?: any) => void;
 interface NativeFiltersFormItem {
   scope: Scope;
   name: string;
+  filterType: FilterType;
   dataset: {
     value: number;
     label: string;
   };
   column: string;
-  defaultValue: string;
+  defaultValue: any;

Review comment:
       Should this be `CurrentFilterState`?

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/state.ts
##########
@@ -113,17 +136,70 @@ export function useFilterScopeTree(): {
 }
 
 export function useCascadingFilters(id: string) {
-  return useSelector<any, ExtraFormData>(state => {
-    const { nativeFilters }: { nativeFilters: NativeFiltersState } = state;
-    const { filters, filtersState } = nativeFilters;
-    const filter = filters[id];
-    const cascadeParentIds = filter?.cascadeParentIds ?? [];
-    let cascadedFilters = {};
-    cascadeParentIds.forEach(parentId => {
-      const parentState = filtersState[parentId] || {};
-      const { extraFormData: parentExtra = {} } = parentState;
-      cascadedFilters = mergeExtraFormData(cascadedFilters, parentExtra);
-    });
-    return cascadedFilters;
+  const nativeFilters = useSelector<any, NativeFiltersState>(
+    state => state.nativeFilters,
+  );
+  const { filters, filtersState } = nativeFilters;
+  const filter = filters[id];
+  const cascadeParentIds = filter?.cascadeParentIds ?? [];
+  let cascadedFilters = {};
+  cascadeParentIds.forEach(parentId => {
+    const parentState = filtersState[parentId] || {};
+    const { extraFormData: parentExtra = {} } = parentState;
+    cascadedFilters = mergeExtraFormData(cascadedFilters, parentExtra);
   });
+  return cascadedFilters;
 }
+
+// When some fields in form changed we need re-fetch data for Filter defaultValue
+export const useBEFormUpdate = (

Review comment:
       This could be named more descriptively, maybe `useBackendFormUpdate`.

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/types.ts
##########
@@ -78,15 +82,15 @@ export type FilterType = 'text' | 'date';
 export interface Filter {
   allowsMultipleValues: boolean;
   cascadeParentIds: string[];
-  defaultValue: string | null;
-  currentValue?: (string | number | boolean)[] | null;
+  defaultValue: any;
+  currentValue?: any;

Review comment:
       Same here

##########
File path: superset-frontend/src/filters/components/Select/types.ts
##########
@@ -45,7 +46,8 @@ export type AntdPluginFilterSelectProps = AntdPluginFilterStylesProps & {
 };
 
 export const DEFAULT_FORM_DATA: AntdPluginFilterSelectCustomizeProps = {
-  defaultValues: [],
+  defaultValue: [],
+  currentValue: [],

Review comment:
       Should these default to `null`?

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/CascadePopover.tsx
##########
@@ -22,14 +22,19 @@ import Popover from 'src/common/components/Popover';
 import Icon from 'src/components/Icon';
 import { Pill } from 'src/dashboard/components/FiltersBadge/Styles';
 import { CascadeFilterControl, FilterControl } from './FilterBar';
-import { Filter, CascadeFilter } from './types';
+import { Filter, CascadeFilter, CurrentFilterState } from './types';
+import { useFilterState } from './state';
 
 interface CascadePopoverProps {
   filter: CascadeFilter;
   visible: boolean;
   directPathToChild?: string[];
   onVisibleChange: (visible: boolean) => void;
-  onExtraFormDataChange: (filter: Filter, extraFormData: ExtraFormData) => void;
+  onExtraFormDataChange: (
+    filter: Filter,
+    extraFormData: ExtraFormData,
+    currentState: CurrentFilterState,
+  ) => void;

Review comment:
       we need to think of a better name for this now that the scope has changed. `onFilterSelectionChange`?

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/state.ts
##########
@@ -27,14 +27,24 @@ import {
   CHART_TYPE,
   DASHBOARD_ROOT_TYPE,
 } from 'src/dashboard/util/componentTypes';
+import { FormInstance } from 'antd/lib/form';
 import {
+  CurrentFilterState,
   Filter,
   FilterConfiguration,
   FilterState,
+  NativeFiltersForm,
   NativeFiltersState,
   TreeItem,
 } from './types';
-import { buildTree, mergeExtraFormData } from './utils';
+import {
+  buildTree,
+  getFormData,
+  mergeExtraFormData,
+  setFilterFieldValues,
+  useForceUpdate,
+} from './utils';
+import { getChartDataRequest } from '../../../chart/chartAction';

Review comment:
       nit: 
   ```suggestion
   import { getChartDataRequest } from 'src/chart/chartAction';
   ```

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/types.ts
##########
@@ -99,11 +103,15 @@ export interface CascadeFilter extends Filter {
 export type FilterConfiguration = Filter[];
 
 export type SelectedValues = string[] | null;
+export type CurrentFilterState = {
+  value: any;
+};

Review comment:
       `CurrentFilterState` could be an alias for `JsonObject` from `@superset-ui/core` for now (making it an alias makes it easier for us to make this more specific in the future if needed).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org