[GitHub] [superset] villebro commented on a change in pull request #13021: refactor(native-filters): decouple params from filter config modal (first phase)

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

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



##########
File path: superset-frontend/src/filters/components/Range/controlPanel.ts
##########
@@ -16,29 +16,10 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { t, validateNonEmpty } from '@superset-ui/core';
-import { ControlPanelConfig, sections } from '@superset-ui/chart-controls';
+import { ControlPanelConfig } from '@superset-ui/chart-controls';
 
 const config: ControlPanelConfig = {
-  // For control input types, see: superset-frontend/src/explore/components/controls/index.js
-  controlPanelSections: [
-    // @ts-ignore
-    sections.legacyRegularTime,
-    {
-      label: t('Query'),
-      expanded: true,
-      controlSetRows: [['groupby'], ['adhoc_filters']],
-    },
-  ],
-  controlOverrides: {
-    groupby: {
-      validators: [validateNonEmpty],
-      clearable: false,
-    },
-    row_limit: {
-      default: 100,
-    },
-  },
+  controlPanelSections: [],

Review comment:
       I think we should leave legacyRegularTime (the others can be removed). Also, let's add a `// TODO:` here to add the relevant controls (I can do that in a follow-up PR).

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterConfigModal/utils.ts
##########
@@ -175,6 +177,30 @@ export const setFilterFieldValues = (
   });
 };
 
+const secondaryFields = ['defaultValueQueriesData', 'defaultValueFormData'];
+
+export const filterOutSecondaryFields = (formInputs: NativeFiltersFormItem) =>
+  Object.entries(formInputs).reduce((resultInFormInputs, [key, value]) => {
+    if (!secondaryFields.includes(key)) {
+      return {
+        ...resultInFormInputs,
+        [key]: value,
+      };
+    }
+    return resultInFormInputs;
+  }, {} as NativeFiltersFormItem);

Review comment:
       To avoid having to do this, would it make sense to introduce a new prop `controlValues` in which we place all control values that are specific to the filter at hand? In the case of the Select filter, it would then contain `enableEmptyFilter`, `inverseSelection` and `multiSelect`. It would also minimize the risk of name collision.

##########
File path: superset-frontend/src/filters/components/Select/controlPanel.ts
##########
@@ -35,17 +29,21 @@ const config: ControlPanelConfig = {
     {
       label: t('Query'),
       expanded: true,
+      controlSetRows: [['groupby']],
+    },
+    {
+      label: t('UI Configuration'),
+      expanded: true,
       controlSetRows: [
-        ['groupby'],
-        ['metrics'],
-        ['adhoc_filters'],
         [
           {
             name: 'multiSelect',
             config: {
               type: 'CheckboxControl',
               label: t('Multiple select'),
               default: multiSelect,
+              resetConfig: true,
+              renderTrigger: true,

Review comment:
       Filtering on only renderTrigger might be fragile. But we can probably come up with a better option for this in a follow-up PR.




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