You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by yo...@apache.org on 2022/04/06 12:25:41 UTC
[superset] branch master updated: fix(explore): clean data when hidding control (#19039)
This is an automated email from the ASF dual-hosted git repository.
yongjiezhao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 0e29871493 fix(explore): clean data when hidding control (#19039)
0e29871493 is described below
commit 0e29871493171b6a70f974d26f41b6797e5b5d5c
Author: Stephen Liu <75...@qq.com>
AuthorDate: Wed Apr 6 20:25:32 2022 +0800
fix(explore): clean data when hidding control (#19039)
---
.../src/shared-controls/index.tsx | 4 +-
.../src/explore/components/Control.test.tsx | 94 ++++++++++++++++++++++
.../src/explore/components/Control.tsx | 29 ++++++-
.../explore/components/ControlPanelsContainer.tsx | 9 ++-
.../src/explore/components/ControlRow.test.tsx | 45 +++++++++--
.../src/explore/components/ControlRow.tsx | 18 +++--
6 files changed, 179 insertions(+), 20 deletions(-)
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx
index edd6e30b02..ec50568315 100644
--- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx
+++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx
@@ -34,6 +34,7 @@
* control interface.
*/
import React from 'react';
+import { isEmpty } from 'lodash';
import {
FeatureFlag,
t,
@@ -43,7 +44,6 @@ import {
SequentialScheme,
legacyValidateInteger,
validateNonEmpty,
- JsonArray,
ComparisionType,
} from '@superset-ui/core';
@@ -352,7 +352,7 @@ const order_desc: SharedControlConfig<'CheckboxControl'> = {
visibility: ({ controls }) =>
Boolean(
controls?.timeseries_limit_metric.value &&
- (controls?.timeseries_limit_metric.value as JsonArray).length,
+ !isEmpty(controls?.timeseries_limit_metric.value),
),
};
diff --git a/superset-frontend/src/explore/components/Control.test.tsx b/superset-frontend/src/explore/components/Control.test.tsx
new file mode 100644
index 0000000000..3921d43746
--- /dev/null
+++ b/superset-frontend/src/explore/components/Control.test.tsx
@@ -0,0 +1,94 @@
+/**
+ * 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 { mount } from 'enzyme';
+import {
+ ThemeProvider,
+ supersetTheme,
+ promiseTimeout,
+} from '@superset-ui/core';
+import React from 'react';
+import { render, screen } from 'spec/helpers/testing-library';
+import Control, { ControlProps } from 'src/explore/components/Control';
+
+const defaultProps: ControlProps = {
+ type: 'CheckboxControl',
+ name: 'checkbox',
+ value: true,
+ actions: {
+ setControlValue: jest.fn(),
+ },
+};
+
+const setup = (overrides = {}) => (
+ <ThemeProvider theme={supersetTheme}>
+ <Control {...defaultProps} {...overrides} />
+ </ThemeProvider>
+);
+
+describe('Control', () => {
+ it('render a control', () => {
+ render(setup());
+
+ const checkbox = screen.getByRole('checkbox');
+ expect(checkbox).toBeVisible();
+ });
+
+ it('render null if type is not exit', () => {
+ render(
+ setup({
+ type: undefined,
+ }),
+ );
+ expect(screen.queryByRole('checkbox')).not.toBeInTheDocument();
+ });
+
+ it('render null if type is not valid', () => {
+ render(
+ setup({
+ type: 'UnkownControl',
+ }),
+ );
+ expect(screen.queryByRole('checkbox')).not.toBeInTheDocument();
+ });
+
+ it('render null if isVisible is false', () => {
+ render(
+ setup({
+ isVisible: false,
+ }),
+ );
+ expect(screen.queryByRole('checkbox')).not.toBeInTheDocument();
+ });
+
+ it('call setControlValue if isVisible is false', () => {
+ const wrapper = mount(
+ setup({
+ isVisible: true,
+ default: false,
+ }),
+ );
+ wrapper.setProps({
+ isVisible: false,
+ default: false,
+ });
+ promiseTimeout(() => {
+ expect(defaultProps.actions.setControlValue).toBeCalled();
+ }, 100);
+ });
+});
diff --git a/superset-frontend/src/explore/components/Control.tsx b/superset-frontend/src/explore/components/Control.tsx
index a8ade357da..804c3d6b10 100644
--- a/superset-frontend/src/explore/components/Control.tsx
+++ b/superset-frontend/src/explore/components/Control.tsx
@@ -16,12 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { ReactNode, useCallback, useState } from 'react';
+import React, { ReactNode, useCallback, useState, useEffect } from 'react';
+import { isEqual } from 'lodash';
import {
ControlType,
ControlComponentProps as BaseControlComponentProps,
} from '@superset-ui/chart-controls';
import { styled, JsonValue, QueryFormData } from '@superset-ui/core';
+import { usePrevious } from 'src/hooks/usePrevious';
import ErrorBoundary from 'src/components/ErrorBoundary';
import { ExploreActions } from 'src/explore/actions/exploreActions';
import controlMap from './controls';
@@ -42,6 +44,8 @@ export type ControlProps = {
validationErrors?: any[];
hidden?: boolean;
renderTrigger?: boolean;
+ default?: JsonValue;
+ isVisible?: boolean;
};
/**
@@ -60,15 +64,36 @@ export default function Control(props: ControlProps) {
name,
type,
hidden,
+ isVisible,
} = props;
const [hovered, setHovered] = useState(false);
+ const wasVisible = usePrevious(isVisible);
const onChange = useCallback(
(value: any, errors: any[]) => setControlValue(name, value, errors),
[name, setControlValue],
);
- if (!type) return null;
+ useEffect(() => {
+ if (
+ wasVisible === true &&
+ isVisible === false &&
+ props.default !== undefined &&
+ !isEqual(props.value, props.default)
+ ) {
+ // reset control value if setting to invisible
+ setControlValue?.(name, props.default);
+ }
+ }, [
+ name,
+ wasVisible,
+ isVisible,
+ setControlValue,
+ props.value,
+ props.default,
+ ]);
+
+ if (!type || isVisible === false) return null;
const ControlComponent = typeof type === 'string' ? controlMap[type] : type;
if (!ControlComponent) {
diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
index 650e5f00c0..832d3552f6 100644
--- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
+++ b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
@@ -283,16 +283,17 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
validationErrors?: any[];
};
- // if visibility check says the config is not visible, don't render it
- if (visibility && !visibility.call(config, props, controlData)) {
- return null;
- }
+ const isVisible = visibility
+ ? visibility.call(config, props, controlData)
+ : undefined;
+
return (
<Control
key={`control-${name}`}
name={name}
validationErrors={validationErrors}
actions={props.actions}
+ isVisible={isVisible}
{...restProps}
/>
);
diff --git a/superset-frontend/src/explore/components/ControlRow.test.tsx b/superset-frontend/src/explore/components/ControlRow.test.tsx
index 638b6772d7..0b57078676 100644
--- a/superset-frontend/src/explore/components/ControlRow.test.tsx
+++ b/superset-frontend/src/explore/components/ControlRow.test.tsx
@@ -17,20 +17,51 @@
* under the License.
*/
import React from 'react';
-import { render } from 'spec/helpers/testing-library';
+import { render, screen } from 'spec/helpers/testing-library';
import ControlSetRow from 'src/explore/components/ControlRow';
+const MockControl = (props: {
+ children: React.ReactElement;
+ type?: string;
+ isVisible?: boolean;
+}) => <div>{props.children}</div>;
describe('ControlSetRow', () => {
it('renders a single row with one element', () => {
- const { getAllByText } = render(
- <ControlSetRow controls={[<p>My Control 1</p>]} />,
- );
- expect(getAllByText('My Control 1').length).toBe(1);
+ render(<ControlSetRow controls={[<p>My Control 1</p>]} />);
+ expect(screen.getAllByText('My Control 1').length).toBe(1);
});
it('renders a single row with two elements', () => {
- const { getAllByText } = render(
+ render(
<ControlSetRow controls={[<p>My Control 1</p>, <p>My Control 2</p>]} />,
);
- expect(getAllByText(/My Control/)).toHaveLength(2);
+ expect(screen.getAllByText(/My Control/)).toHaveLength(2);
+ });
+
+ it('renders a single row with one elements if is HiddenControl', () => {
+ render(
+ <ControlSetRow
+ controls={[
+ <p>My Control 1</p>,
+ <MockControl type="HiddenControl">
+ <p>My Control 2</p>
+ </MockControl>,
+ ]}
+ />,
+ );
+ expect(screen.getAllByText(/My Control/)).toHaveLength(2);
+ });
+
+ it('renders a single row with one elements if is invisible', () => {
+ render(
+ <ControlSetRow
+ controls={[
+ <p>My Control 1</p>,
+ <MockControl isVisible={false}>
+ <p>My Control 2</p>
+ </MockControl>,
+ ]}
+ />,
+ );
+ expect(screen.getAllByText(/My Control/)).toHaveLength(2);
});
});
diff --git a/superset-frontend/src/explore/components/ControlRow.tsx b/superset-frontend/src/explore/components/ControlRow.tsx
index 4a1dfd5789..5721b5de28 100644
--- a/superset-frontend/src/explore/components/ControlRow.tsx
+++ b/superset-frontend/src/explore/components/ControlRow.tsx
@@ -16,26 +16,34 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React from 'react';
+import React, { useCallback } from 'react';
const NUM_COLUMNS = 12;
type Control = React.ReactElement | null;
export default function ControlRow({ controls }: { controls: Control[] }) {
- // ColorMapControl renders null and should not be counted
+ const isHiddenControl = useCallback(
+ (control: Control) =>
+ control?.props.type === 'HiddenControl' ||
+ control?.props.isVisible === false,
+ [],
+ );
+ // Invisible control should not be counted
// in the columns number
const countableControls = controls.filter(
- control => !['ColorMapControl'].includes(control?.props.type),
+ control => !isHiddenControl(control),
);
- const colSize = NUM_COLUMNS / countableControls.length;
+ const colSize = countableControls.length
+ ? NUM_COLUMNS / countableControls.length
+ : NUM_COLUMNS;
return (
<div className="row">
{controls.map((control, i) => (
<div
className={`col-lg-${colSize} col-xs-12`}
style={{
- display: control?.props.type === 'HiddenControl' ? 'none' : 'block',
+ display: isHiddenControl(control) ? 'none' : 'block',
}}
key={i}
>