villebro commented on a change in pull request #13437:
URL: https://github.com/apache/superset/pull/13437#discussion_r587607886
##########
File path: superset-frontend/spec/fixtures/mockNativeFilters.ts
##########
@@ -132,14 +134,15 @@ export const singleNativeFiltersState = {
isRequired: false,
},
},
- filtersState: {
- nativeFilters: {
- [NATIVE_FILTER_ID]: {
- id: NATIVE_FILTER_ID,
- extraFormData,
- currentState: {
- value: ['No, not an ethnic minority'],
- },
+};
+
+export const singleDataMask = {
Review comment:
Also this name could be more descriptive. Also can we add the type here?
##########
File path: superset-frontend/src/dashboard/reducers/index.js
##########
@@ -19,6 +19,7 @@
import { combineReducers } from 'redux';
import charts from '../../chart/chartReducer';
+import dataMask from '../../dataMask/reducer';
Review comment:
```suggestion
import charts from 'src/chart/chartReducer';
import dataMask from 'src/dataMask/reducer';
```
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CascadePopover.tsx
##########
@@ -21,18 +21,20 @@ import { styled, t, DataMask } from '@superset-ui/core';
import Popover from 'src/common/components/Popover';
import Icon from 'src/components/Icon';
import { Pill } from 'src/dashboard/components/FiltersBadge/Styles';
-import { useFilterStateNative } from './state';
+import { useSelector } from 'react-redux';
import FilterControl from './FilterControl';
import CascadeFilterControl from './CascadeFilterControl';
import { CascadeFilter } from './types';
import { Filter } from '../types';
+import { getInitialMask } from '../../../../dataMask/reducer';
+import { MaskWithId } from '../../../../dataMask/types';
Review comment:
```suggestion
import { getInitialMask } from 'src/dataMask/reducer';
import { MaskWithId } from 'src/dataMask/types';
```
##########
File path:
superset-frontend/spec/javascripts/dashboard/fixtures/mockNativeFilters.js
##########
@@ -41,13 +41,14 @@ export const nativeFiltersInfo = {
isRequired: false,
},
},
- filtersState: {
- nativeFilters: {
- DefaultsID: {
- id: 'DefaultId',
- currentState: {
- value: [],
- },
+};
+
+export const mockDataMaskInfo = {
Review comment:
I wonder why this is not TS, could we do that while we're at it?
##########
File path: superset-frontend/src/dashboard/actions/nativeFilters.ts
##########
@@ -17,16 +17,16 @@
* under the License.
*/
-import { makeApi, DataMask } from '@superset-ui/core';
+import { makeApi } from '@superset-ui/core';
import { Dispatch } from 'redux';
import { FilterConfiguration } from
'src/dashboard/components/nativeFilters/types';
import { dashboardInfoChanged } from './dashboardInfo';
+import { FiltersSet } from '../reducers/types';
+import { DataMaskType, DataMaskStateWithId } from '../../dataMask/types';
import {
- FiltersState,
- FilterState,
- FiltersSet,
- FilterStateType,
-} from '../reducers/types';
+ SET_DATA_MASK_FOR_FILTER_CONFIG_COMPLETE,
+ SET_DATA_MASK_FOR_FILTER_CONFIG_FAIL,
+} from '../../dataMask/actions';
Review comment:
```suggestion
} from 'src/dataMask/actions';
```
##########
File path: superset-frontend/src/dashboard/reducers/types.ts
##########
@@ -18,8 +18,8 @@
*/
import componentTypes from 'src/dashboard/util/componentTypes';
-import { ExtraFormData, DataMaskCurrentState } from '@superset-ui/core';
import { Filter } from '../components/nativeFilters/types';
+import { DataMaskStateWithId } from '../../dataMask/types';
Review comment:
```suggestion
import { Filter } from '../components/nativeFilters/types';
import { DataMaskStateWithId } from 'src/dataMask/types';
```
##########
File path: superset-frontend/src/dashboard/actions/nativeFilters.ts
##########
@@ -17,16 +17,16 @@
* under the License.
*/
-import { makeApi, DataMask } from '@superset-ui/core';
+import { makeApi } from '@superset-ui/core';
import { Dispatch } from 'redux';
import { FilterConfiguration } from
'src/dashboard/components/nativeFilters/types';
import { dashboardInfoChanged } from './dashboardInfo';
+import { FiltersSet } from '../reducers/types';
+import { DataMaskType, DataMaskStateWithId } from '../../dataMask/types';
Review comment:
nit
```suggestion
import { DataMaskType, DataMaskStateWithId } from 'src/dataMask/types';
```
##########
File path: superset-frontend/src/dataMask/actions.ts
##########
@@ -0,0 +1,62 @@
+/**
Review comment:
I see the reasoning behind having `dataMask` on the root level, as they
will be used in viz, filters, dashboard and explore, but I still feel
`superset-frontend/src/dashboard/dataMask` might be more appropriate, as that's
where these masks are primarily used.
##########
File path: superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
##########
@@ -21,6 +21,7 @@ import { getChartIdsInFilterScope } from
'src/dashboard/util/activeDashboardFilt
import { NativeFiltersState } from 'src/dashboard/reducers/types';
import { Layout } from '../../types';
import { getTreeCheckedItems } from
'../nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils';
+import { DataMaskStateWithId } from '../../../dataMask/types';
Review comment:
```suggestion
import { DataMaskStateWithId } from 'src/dataMask/types';
```
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -54,6 +54,7 @@ import {
LOG_ACTIONS_MOUNT_EXPLORER,
LOG_ACTIONS_CHANGE_EXPLORE_CONTROLS,
} from '../../logger/LogUtils';
+import { mergeExtraFormData } from
'../../dashboard/components/nativeFilters/utils';
Review comment:
```suggestion
import { mergeExtraFormData } from
'src/dashboard/components/nativeFilters/utils';
```
##########
File path: superset-frontend/spec/fixtures/mockNativeFilters.ts
##########
@@ -72,33 +73,34 @@ export const nativeFilters: NativeFiltersState = {
isInstant: true,
},
},
- filtersState: {
- crossFilters: {},
- ownFilters: {},
- nativeFilters: {
- 'NATIVE_FILTER-e7Q8zKixx': {
- id: 'NATIVE_FILTER-e7Q8zKixx',
- extraFormData: {
- append_form_data: {
- filters: [
- {
- col: 'region',
- op: 'IN',
- val: ['East Asia & Pacific'],
- },
- ],
- },
- },
- currentState: {
- value: ['East Asia & Pacific'],
+};
+
+export const dataMask: DataMaskStateWithId = {
Review comment:
Could we make this name more descriptive?
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
