You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/03/04 14:35:38 UTC

[GitHub] [superset] simcha90 opened a new pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root 

simcha90 opened a new pull request #13437:
URL: https://github.com/apache/superset/pull/13437


   ### SUMMARY
   This PR move `(dashboard redux state) -> nativeFilters -> filtersState` to `redux -> dataMask` because of next reasons:
   1. it has now not only nativeFilters data but also cross-filters data and own charts data
   2. we also use it in explore for reusage
   3. it uses `dataMask` naming instead of `filtersState`
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   
   
   https://user-images.githubusercontent.com/56388545/109941833-3a700000-7cdc-11eb-89e8-9503a1e52dd5.mov
   
   
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


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

[GitHub] [superset] villebro commented on pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #13437:
URL: https://github.com/apache/superset/pull/13437#issuecomment-790888678


   FYI @simcha90 this needs a rebase (also left a few comments)


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

[GitHub] [superset] codecov[bot] commented on pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #13437:
URL: https://github.com/apache/superset/pull/13437#issuecomment-792228981


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13437?src=pr&el=h1) Report
   > Merging [#13437](https://codecov.io/gh/apache/superset/pull/13437?src=pr&el=desc) (9d80fe1) into [master](https://codecov.io/gh/apache/superset/commit/d0714a097294f5ab1405b7958994eab1b6af7dec?el=desc) (d0714a0) will **decrease** coverage by `4.03%`.
   > The diff coverage is `86.17%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13437/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13437?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13437      +/-   ##
   ==========================================
   - Coverage   77.41%   73.37%   -4.04%     
   ==========================================
     Files         909      611     -298     
     Lines       45863    21336   -24527     
     Branches     5534     5537       +3     
   ==========================================
   - Hits        35503    15656   -19847     
   + Misses      10232     5550    -4682     
   - Partials      128      130       +2     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `57.97% <86.25%> (+0.05%)` | :arrow_up: |
   | hive | `?` | |
   | javascript | `63.26% <50.00%> (-0.02%)` | :arrow_down: |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `?` | |
   | python | `?` | |
   | sqlite | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13437?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [...s/nativeFilters/FilterBar/CascadeFilterControl.tsx](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0Nhc2NhZGVGaWx0ZXJDb250cm9sLnRzeA==) | `85.71% <ø> (ø)` | |
   | [...perset-frontend/src/dashboard/containers/Chart.jsx](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0NoYXJ0LmpzeA==) | `100.00% <ø> (ø)` | |
   | [...frontend/src/dashboard/containers/FiltersBadge.tsx](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0ZpbHRlcnNCYWRnZS50c3g=) | `89.28% <ø> (ø)` | |
   | [superset-frontend/src/dashboard/reducers/index.js](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9yZWR1Y2Vycy9pbmRleC5qcw==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/dashboard/reducers/types.ts](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9yZWR1Y2Vycy90eXBlcy50cw==) | `0.00% <ø> (-100.00%)` | :arrow_down: |
   | [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `82.60% <ø> (ø)` | |
   | [superset-frontend/src/explore/reducers/index.js](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvaW5kZXguanM=) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `76.31% <33.33%> (+0.31%)` | :arrow_up: |
   | [superset-frontend/src/chart/chartAction.js](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L2NoYXJ0QWN0aW9uLmpz) | `78.00% <50.00%> (ø)` | |
   | ... and [319 more](https://codecov.io/gh/apache/superset/pull/13437/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13437?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13437?src=pr&el=footer). Last update [d0714a0...2d12c37](https://codecov.io/gh/apache/superset/pull/13437?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [superset] simcha90 commented on a change in pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
simcha90 commented on a change in pull request #13437:
URL: https://github.com/apache/superset/pull/13437#discussion_r588980235



##########
File path: superset-frontend/src/dataMask/actions.ts
##########
@@ -0,0 +1,62 @@
+/**

Review comment:
       I think it's the same reason why we put `chart` in the root, it's parallel to chart I think... Also this feature have enough functionality to have independent place in root :)




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

[GitHub] [superset] villebro commented on a change in pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org



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

[GitHub] [superset] simcha90 commented on a change in pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
simcha90 commented on a change in pull request #13437:
URL: https://github.com/apache/superset/pull/13437#discussion_r587366278



##########
File path: superset-frontend/src/dataMask/types.ts
##########
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ExtraFormData, DataMaskCurrentState } from '@superset-ui/core';
+
+export enum DataMaskType {
+  NativeFilters = 'nativeFilters',
+  CrossFilters = 'crossFilters',
+  OwnFilters = 'ownFilters',
+}
+
+export type SingleMask = {
+  extraFormData?: ExtraFormData;
+  currentState: DataMaskCurrentState;
+};
+
+export type MultipleMask = SingleMask & { id: string };
+
+export type MultipleDataMask = { [filterId: string]: MultipleMask };
+export type MultipleDataMaskState = {
+  [DataMaskType.NativeFilters]: MultipleDataMask;
+  [DataMaskType.CrossFilters]: MultipleDataMask;
+  [DataMaskType.OwnFilters]: MultipleDataMask;
+};
+
+export type SingleDataMask = { [filterId: string]: SingleMask };
+export type SingleDataMaskState = {
+  [DataMaskType.NativeFilters]: SingleMask;
+  [DataMaskType.CrossFilters]: SingleMask;
+  [DataMaskType.OwnFilters]: SingleMask;
+};

Review comment:
       A little bit changed on other names




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

[GitHub] [superset] villebro merged pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
villebro merged pull request #13437:
URL: https://github.com/apache/superset/pull/13437


   


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

[GitHub] [superset] villebro commented on a change in pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13437:
URL: https://github.com/apache/superset/pull/13437#discussion_r587326487



##########
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 datMask: MultipleDataMaskState = {

Review comment:
       typo: `datMask`

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CascadePopover.tsx
##########
@@ -80,7 +82,10 @@ const CascadePopover: React.FC<CascadePopoverProps> = ({
   directPathToChild,
 }) => {
   const [currentPathToChild, setCurrentPathToChild] = useState<string[]>();
-  const filterStateNative = useFilterStateNative(filter.id);
+  const filterStateNative = useSelector<any, MultipleMask>(
+    state =>
+      state.dataMask.nativeFilters[filter.id] ?? getInitialMask(filter.id),
+  );

Review comment:
       👍 

##########
File path: superset-frontend/src/dataMask/types.ts
##########
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ExtraFormData, DataMaskCurrentState } from '@superset-ui/core';
+
+export enum DataMaskType {
+  NativeFilters = 'nativeFilters',
+  CrossFilters = 'crossFilters',
+  OwnFilters = 'ownFilters',
+}
+
+export type SingleMask = {
+  extraFormData?: ExtraFormData;
+  currentState: DataMaskCurrentState;
+};
+
+export type MultipleMask = SingleMask & { id: string };
+
+export type MultipleDataMask = { [filterId: string]: MultipleMask };
+export type MultipleDataMaskState = {
+  [DataMaskType.NativeFilters]: MultipleDataMask;
+  [DataMaskType.CrossFilters]: MultipleDataMask;
+  [DataMaskType.OwnFilters]: MultipleDataMask;
+};
+
+export type SingleDataMask = { [filterId: string]: SingleMask };
+export type SingleDataMaskState = {
+  [DataMaskType.NativeFilters]: SingleMask;
+  [DataMaskType.CrossFilters]: SingleMask;
+  [DataMaskType.OwnFilters]: SingleMask;
+};

Review comment:
       These feel slightly confusing. I think something like this could help clarify for the casual developer that stumbles into this area of the code:
   
   - Instead of `SingleMask` we would have `DataMask`
   - Instead of `MultipleDataMask` we would have `DataMaskWithId`
   - Instead of `MultipleDataMaskState` we would have `DataMaskStateWithId` (not thrilled about this name, but it's still slightly more descriptive IMO)
   - Instead of `SingleDataMaskState` we would then have `DataMaskState`




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

[GitHub] [superset] simcha90 commented on a change in pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
simcha90 commented on a change in pull request #13437:
URL: https://github.com/apache/superset/pull/13437#discussion_r588980235



##########
File path: superset-frontend/src/dataMask/actions.ts
##########
@@ -0,0 +1,62 @@
+/**

Review comment:
       I think it's the same reason why we put `chart` in the root, it's parallel to chart I think... Also this feature have enough functionality to have independent place in root :) Also I think in future we will need to add here also `ui filtering` like ui changes that will applied to other charts, may be also we will need to use here to show how it's applied to itself




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

[GitHub] [superset] simcha90 commented on a change in pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
simcha90 commented on a change in pull request #13437:
URL: https://github.com/apache/superset/pull/13437#discussion_r588979054



##########
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:
       for example? I also not sure that it's good to add to complexity to name, because in the code we also add prefixes/postfixes, function name to this basic name...




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

[GitHub] [superset] simcha90 closed pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
simcha90 closed pull request #13437:
URL: https://github.com/apache/superset/pull/13437


   


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

[GitHub] [superset] ktmud commented on a change in pull request #13437: refactor(native-filters): Move `filtersState` to `dataMask` redux root

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13437:
URL: https://github.com/apache/superset/pull/13437#discussion_r589082249



##########
File path: superset-frontend/package.json
##########
@@ -112,6 +112,7 @@
     "geolib": "^2.0.24",
     "global-box": "^1.2.0",
     "html-webpack-plugin": "^4.5.1",
+    "immer": "^8.0.1",

Review comment:
       When adding a new library, it would be nice to add a note in PR description to explain why it is needed. Sometimes it's not very obvious what the purpose of the library is.




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