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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
