fix: Fixes #12672 (#14525)

superset-frontend/spec/javascripts/explore/components/BoundsControl_spec.jsx

@@ -17,47 +17,37 @@
  * under the License.
  */
 import React from 'react';
-import sinon from 'sinon';
-import { styledMount as mount } from 'spec/helpers/theming';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
 import BoundsControl from 'src/explore/components/controls/BoundsControl';
-import { Input } from 'src/common/components';
 
 const defaultProps = {
   name: 'y_axis_bounds',
   label: 'Bounds of the y axis',
-  onChange: sinon.spy(),
+  onChange: jest.fn(),
 };
 
-describe('BoundsControl', () => {
-  let wrapper;
-
-  beforeEach(() => {
-    wrapper = mount(<BoundsControl {...defaultProps} />);
-  });
-
-  it('renders two Input', () => {
-    expect(wrapper.find(Input)).toHaveLength(2);
-  });
-
-  it('errors on non-numeric', () => {
-    wrapper
-      .find(Input)
-      .first()
-      .simulate('change', { target: { value: 's' } });
-    expect(defaultProps.onChange.calledWith([null, null])).toBe(true);
-    expect(defaultProps.onChange.getCall(0).args[1][0]).toContain(
-      'value should be numeric',
-    );
-  });
-  it('casts to numeric', () => {
-    wrapper
-      .find(Input)
-      .first()
-      .simulate('change', { target: { value: '1' } });
-    wrapper
-      .find(Input)
-      .last()
-      .simulate('change', { target: { value: '5' } });
-    expect(defaultProps.onChange.calledWith([1, 5])).toBe(true);
-  });
+test('renders two inputs', () => {
+  render(<BoundsControl {...defaultProps} />);
+  expect(screen.getAllByRole('spinbutton')).toHaveLength(2);
+});
+
+test('receives null on non-numeric', async () => {
+  render(<BoundsControl {...defaultProps} />);
+  const minInput = screen.getAllByRole('spinbutton')[0];
+  userEvent.type(minInput, 'text');
+  await waitFor(() =>
+    expect(defaultProps.onChange).toHaveBeenCalledWith([null, null]),
+  );
+});
+
+test('calls onChange with correct values', async () => {
+  render(<BoundsControl {...defaultProps} />);
+  const minInput = screen.getAllByRole('spinbutton')[0];
+  const maxInput = screen.getAllByRole('spinbutton')[1];
+  userEvent.type(minInput, '1');
+  userEvent.type(maxInput, '2');
+  await waitFor(() =>
+    expect(defaultProps.onChange).toHaveBeenLastCalledWith([1, 2]),
+  );
 });

superset-frontend/src/common/components/index.tsx

@@ -18,7 +18,13 @@
  */
 import React from 'react';
 import { styled } from '@superset-ui/core';
-import { Dropdown, Menu as AntdMenu, Input as AntdInput, Skeleton } from 'antd';
+import {
+  Dropdown,
+  Menu as AntdMenu,
+  Input as AntdInput,
+  InputNumber as AntdInputNumber,
+  Skeleton,
+} from 'antd';
 import { DropDownProps } from 'antd/lib/dropdown';
 /*
   Antd is re-exported from here so we can override components with Emotion as needed.
@@ -36,7 +42,6 @@ export {
   Dropdown,
   Form,
   Empty,
-  InputNumber,
   Modal,
   Typography,
   Tree,
@@ -200,6 +205,11 @@ export const Input = styled(AntdInput)`
   border-radius: ${({ theme }) => theme.borderRadius}px;
 `;
 
+export const InputNumber = styled(AntdInputNumber)`
+  border: 1px solid ${({ theme }) => theme.colors.secondary.light3};
+  border-radius: ${({ theme }) => theme.borderRadius}px;
+`;
+
 export const TextArea = styled(AntdInput.TextArea)`
   border: 1px solid ${({ theme }) => theme.colors.secondary.light3};
   border-radius: ${({ theme }) => theme.borderRadius}px;

superset-frontend/src/explore/components/controls/BoundsControl.jsx

@@ -18,9 +18,10 @@
  */
 import React from 'react';
 import PropTypes from 'prop-types';
-import { Row, Col, Input } from 'src/common/components';
-import { t } from '@superset-ui/core';
-import ControlHeader from '../ControlHeader';
+import { InputNumber } from 'src/common/components';
+import { t, styled } from '@superset-ui/core';
+import { isEqual, debounce } from 'lodash';
+import ControlHeader from 'src/explore/components/ControlHeader';
 
 const propTypes = {
   onChange: PropTypes.func,
@@ -32,35 +33,63 @@ const defaultProps = {
   value: [null, null],
 };
 
+const StyledDiv = styled.div`
+  display: flex;
+`;
+
+const MinInput = styled(InputNumber)`
+  flex: 1;
+  margin-right: ${({ theme }) => theme.gridUnit}px;
+`;
+
+const MaxInput = styled(InputNumber)`
+  flex: 1;
+  margin-left: ${({ theme }) => theme.gridUnit}px;
+`;
+
 export default class BoundsControl extends React.Component {
   constructor(props) {
     super(props);
     this.state = {
       minMax: [
-        props.value[0] === null ? '' : props.value[0],
-        props.value[1] === null ? '' : props.value[1],
+        Number.isNaN(this.props.value[0]) ? '' : props.value[0],
+        Number.isNaN(this.props.value[1]) ? '' : props.value[1],
       ],
     };
-    this.onChange = this.onChange.bind(this);
+    this.onChange = debounce(this.onChange.bind(this), 300);
     this.onMinChange = this.onMinChange.bind(this);
     this.onMaxChange = this.onMaxChange.bind(this);
+    this.update = this.update.bind(this);
   }
 
-  onMinChange(event) {
-    const min = event.target.value;
+  componentDidUpdate(prevProps) {
+    if (!isEqual(prevProps.value, this.props.value)) {
+      this.update();
+    }
+  }
+
+  update() {
+    this.setState({
+      minMax: [
+        Number.isNaN(this.props.value[0]) ? '' : this.props.value[0],
+        Number.isNaN(this.props.value[1]) ? '' : this.props.value[1],
+      ],
+    });
+  }
+
+  onMinChange(value) {
     this.setState(
       prevState => ({
-        minMax: [min, prevState.minMax[1]],
+        minMax: [value, prevState.minMax[1]],
       }),
       this.onChange,
     );
   }
 
-  onMaxChange(event) {
-    const max = event.target.value;
+  onMaxChange(value) {
     this.setState(
       prevState => ({
-        minMax: [prevState.minMax[0], max],
+        minMax: [prevState.minMax[0], value],
       }),
       this.onChange,
     );
@@ -68,44 +97,29 @@ export default class BoundsControl extends React.Component {
 
   onChange() {
     const mm = this.state.minMax;
-    const errors = [];
-    if (mm[0] && Number.isNaN(Number(mm[0]))) {
-      errors.push(t('`Min` value should be numeric or empty'));
-    }
-    if (mm[1] && Number.isNaN(Number(mm[1]))) {
-      errors.push(t('`Max` value should be numeric or empty'));
-    }
-    if (errors.length === 0) {
-      this.props.onChange([parseFloat(mm[0]), parseFloat(mm[1])], errors);
-    } else {
-      this.props.onChange([null, null], errors);
-    }
+    const min = parseFloat(mm[0]) || null;
+    const max = parseFloat(mm[1]) || null;
+    this.props.onChange([min, max]);
   }
 
   render() {
     return (
       <div>
         <ControlHeader {...this.props} />
-        <Row gutter={16}>
-          <Col xs={12}>
-            <Input
-              data-test="min-bound"
-              type="text"
-              placeholder={t('Min')}
-              onChange={this.onMinChange}
-              value={this.state.minMax[0]}
-            />
-          </Col>
-          <Col xs={12}>
-            <Input
-              type="text"
-              data-test="max-bound"
-              placeholder={t('Max')}
-              onChange={this.onMaxChange}
-              value={this.state.minMax[1]}
-            />
-          </Col>
-        </Row>
+        <StyledDiv>
+          <MinInput
+            data-test="min-bound"
+            placeholder={t('Min')}
+            onChange={this.onMinChange}
+            value={this.state.minMax[0]}
+          />
+          <MaxInput
+            data-test="max-bound"
+            placeholder={t('Max')}
+            onChange={this.onMaxChange}
+            value={this.state.minMax[1]}
+          />
+        </StyledDiv>
       </div>
     );
   }

superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/TimeSeriesColumnControl.test.tsx

@@ -21,6 +21,8 @@ import { render, screen } from 'spec/helpers/testing-library';
 import userEvent from '@testing-library/user-event';
 import TimeSeriesColumnControl from '.';
 
+jest.mock('lodash/debounce', () => jest.fn(fn => fn));
+
 test('renders with default props', () => {
   render(<TimeSeriesColumnControl />);
   expect(screen.getByText('Time series columns')).toBeInTheDocument();
@@ -36,16 +38,6 @@ test('renders popover on edit', () => {
   expect(screen.getByText('Type')).toBeInTheDocument();
 });
 
-test('triggers onChange when type changes', () => {
-  const onChange = jest.fn();
-  render(<TimeSeriesColumnControl onChange={onChange} />);
-  userEvent.click(screen.getByRole('button'));
-  userEvent.click(screen.getByText('Select...'));
-  expect(onChange).not.toHaveBeenCalled();
-  userEvent.click(screen.getByText('Time comparison'));
-  expect(onChange).toHaveBeenCalled();
-});
-
 test('renders time comparison', () => {
   render(<TimeSeriesColumnControl colType="time" />);
   userEvent.click(screen.getByRole('button'));
@@ -82,23 +74,47 @@ test('renders period average', () => {
   expect(screen.getByText('Number format')).toBeInTheDocument();
 });
 
+test('triggers onChange when type changes', () => {
+  const onChange = jest.fn();
+  render(<TimeSeriesColumnControl onChange={onChange} />);
+  userEvent.click(screen.getByRole('button'));
+  userEvent.click(screen.getByText('Select...'));
+  userEvent.click(screen.getByText('Time comparison'));
+  expect(onChange).not.toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(
+    expect.objectContaining({ colType: 'time' }),
+  );
+});
+
 test('triggers onChange when time lag changes', () => {
+  const timeLag = '1';
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="time" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
+  const timeLagInput = screen.getByPlaceholderText('Time Lag');
+  userEvent.clear(timeLagInput);
+  userEvent.type(timeLagInput, timeLag);
   expect(onChange).not.toHaveBeenCalled();
-  userEvent.type(screen.getByPlaceholderText('Time Lag'), '1');
-  expect(onChange).toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ timeLag }));
 });
 
 test('triggers onChange when color bounds changes', () => {
+  const min = 1;
+  const max = 5;
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="time" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
+  const minInput = screen.getByPlaceholderText('Min');
+  const maxInput = screen.getByPlaceholderText('Max');
+  userEvent.type(minInput, min.toString());
+  userEvent.type(maxInput, max.toString());
   expect(onChange).not.toHaveBeenCalled();
-  userEvent.type(screen.getByPlaceholderText('Min'), '1');
-  userEvent.type(screen.getByPlaceholderText('Max'), '10');
-  expect(onChange).toHaveBeenCalledTimes(3);
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenLastCalledWith(
+    expect.objectContaining({ bounds: [min, max] }),
+  );
 });
 
 test('triggers onChange when time type changes', () => {
@@ -106,71 +122,102 @@ test('triggers onChange when time type changes', () => {
   render(<TimeSeriesColumnControl colType="time" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
   userEvent.click(screen.getByText('Select...'));
-  expect(onChange).not.toHaveBeenCalled();
   userEvent.click(screen.getByText('Difference'));
-  expect(onChange).toHaveBeenCalled();
+  expect(onChange).not.toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(
+    expect.objectContaining({ comparisonType: 'diff' }),
+  );
 });
 
 test('triggers onChange when number format changes', () => {
+  const numberFormatString = 'Test format';
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="time" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
+  userEvent.type(
+    screen.getByPlaceholderText('Number format string'),
+    numberFormatString,
+  );
   expect(onChange).not.toHaveBeenCalled();
-  userEvent.type(screen.getByPlaceholderText('Number format string'), 'format');
-  expect(onChange).toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(
+    expect.objectContaining({ d3format: numberFormatString }),
+  );
 });
 
 test('triggers onChange when width changes', () => {
+  const width = '10';
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="spark" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
+  userEvent.type(screen.getByPlaceholderText('Width'), width);
   expect(onChange).not.toHaveBeenCalled();
-  userEvent.type(screen.getByPlaceholderText('Width'), '10');
-  expect(onChange).toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ width }));
 });
 
 test('triggers onChange when height changes', () => {
+  const height = '10';
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="spark" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
+  userEvent.type(screen.getByPlaceholderText('Height'), height);
   expect(onChange).not.toHaveBeenCalled();
-  userEvent.type(screen.getByPlaceholderText('Height'), '10');
-  expect(onChange).toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ height }));
 });
 
 test('triggers onChange when time ratio changes', () => {
+  const timeRatio = '10';
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="spark" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
+  userEvent.type(screen.getByPlaceholderText('Time Ratio'), timeRatio);
   expect(onChange).not.toHaveBeenCalled();
-  userEvent.type(screen.getByPlaceholderText('Time Ratio'), '10');
-  expect(onChange).toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(expect.objectContaining({ timeRatio }));
 });
 
 test('triggers onChange when show Y-axis changes', () => {
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="spark" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
-  expect(onChange).not.toHaveBeenCalled();
   userEvent.click(screen.getByRole('checkbox'));
-  expect(onChange).toHaveBeenCalled();
+  expect(onChange).not.toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(
+    expect.objectContaining({ showYAxis: true }),
+  );
 });
 
 test('triggers onChange when Y-axis bounds changes', () => {
+  const min = 1;
+  const max = 5;
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="spark" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
+  const minInput = screen.getByPlaceholderText('Min');
+  const maxInput = screen.getByPlaceholderText('Max');
+  userEvent.type(minInput, min.toString());
+  userEvent.clear(maxInput);
+  userEvent.type(maxInput, max.toString());
   expect(onChange).not.toHaveBeenCalled();
-  userEvent.type(screen.getByPlaceholderText('Min'), '1');
-  userEvent.type(screen.getByPlaceholderText('Max'), '10');
-  expect(onChange).toHaveBeenCalledTimes(3);
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(
+    expect.objectContaining({ yAxisBounds: [min, max] }),
+  );
 });
 
 test('triggers onChange when date format changes', () => {
+  const dateFormat = 'yy/MM/dd';
   const onChange = jest.fn();
   render(<TimeSeriesColumnControl colType="spark" onChange={onChange} />);
   userEvent.click(screen.getByRole('button'));
+  userEvent.type(screen.getByPlaceholderText('Date format string'), dateFormat);
   expect(onChange).not.toHaveBeenCalled();
-  userEvent.type(screen.getByPlaceholderText('Date format string'), 'yy/MM/dd');
-  expect(onChange).toHaveBeenCalled();
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(onChange).toHaveBeenCalledWith(
+    expect.objectContaining({ dateFormat }),
+  );
 });

superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.jsx

@@ -19,11 +19,11 @@
 import React from 'react';
 import PropTypes from 'prop-types';
 import { Row, Col, Input } from 'src/common/components';
+import Button from 'src/components/Button';
 import Popover from 'src/components/Popover';
 import Select from 'src/components/Select';
 import { t, styled } from '@superset-ui/core';
 import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
-
 import BoundsControl from '../BoundsControl';
 import CheckboxControl from '../CheckboxControl';
 
@@ -76,6 +76,8 @@ const colTypeOptions = [
 
 const StyledRow = styled(Row)`
   margin-top: ${({ theme }) => theme.gridUnit * 2}px;
+  display: flex;
+  align-items: center;
 `;
 
 const StyledCol = styled(Col)`
@@ -88,10 +90,27 @@ const StyledTooltip = styled(InfoTooltipWithTrigger)`
   color: ${({ theme }) => theme.colors.grayscale.light1};
 `;
 
+const ButtonBar = styled.div`
+  margin-top: ${({ theme }) => theme.gridUnit * 5}px;
+  display: flex;
+  justify-content: center;
+`;
+
 export default class TimeSeriesColumnControl extends React.Component {
   constructor(props) {
     super(props);
-    const state = {
+
+    this.onSave = this.onSave.bind(this);
+    this.onClose = this.onClose.bind(this);
+    this.resetState = this.resetState.bind(this);
+    this.initialState = this.initialState.bind(this);
+    this.onPopoverVisibleChange = this.onPopoverVisibleChange.bind(this);
+
+    this.state = this.initialState();
+  }
+
+  initialState() {
+    return {
       label: this.props.label,
       tooltip: this.props.tooltip,
       colType: this.props.colType,
@@ -105,57 +124,73 @@ export default class TimeSeriesColumnControl extends React.Component {
       bounds: this.props.bounds,
       d3format: this.props.d3format,
       dateFormat: this.props.dateFormat,
+      popoverVisible: false,
     };
-    delete state.onChange;
-    this.state = state;
-    this.onChange = this.onChange.bind(this);
   }
 
-  onChange() {
+  resetState() {
+    const initialState = this.initialState();
+    this.setState({ ...initialState });
+  }
+
+  onSave() {
     this.props.onChange(this.state);
+    this.setState({ popoverVisible: false });
+  }
+
+  onClose() {
+    this.resetState();
   }
 
   onSelectChange(attr, opt) {
-    this.setState({ [attr]: opt.value }, this.onChange);
+    this.setState({ [attr]: opt.value });
   }
 
   onTextInputChange(attr, event) {
-    this.setState({ [attr]: event.target.value }, this.onChange);
+    this.setState({ [attr]: event.target.value });
   }
 
   onCheckboxChange(attr, value) {
-    this.setState({ [attr]: value }, this.onChange);
+    this.setState({ [attr]: value });
   }
 
   onBoundsChange(bounds) {
-    this.setState({ bounds }, this.onChange);
+    this.setState({ bounds });
+  }
+
+  onPopoverVisibleChange(popoverVisible) {
+    if (popoverVisible) {
+      this.setState({ popoverVisible });
+    } else {
+      this.resetState();
+    }
   }
 
   onYAxisBoundsChange(yAxisBounds) {
-    this.setState({ yAxisBounds }, this.onChange);
+    this.setState({ yAxisBounds });
   }
 
   textSummary() {
-    return `${this.state.label}`;
+    return `${this.props.label}`;
   }
 
   formRow(label, tooltip, ttLabel, control) {
     return (
       <StyledRow>
-        <StyledCol xs={24} md={10}>
+        <StyledCol xs={24} md={11}>
           {label}
           <StyledTooltip placement="top" tooltip={tooltip} label={ttLabel} />
         </StyledCol>
-        <StyledCol xs={24} md={14}>
+        <Col xs={24} md={13}>
           {control}
-        </StyledCol>
+        </Col>
       </StyledRow>
     );
   }
 
   renderPopover() {
     return (
-      <div id="ts-col-popo" style={{ width: 300 }}>
+      <div id="ts-col-popo" style={{ width: 320 }}>
         {this.formRow(
           'Label',
           'The column header label',
@@ -297,6 +332,19 @@ export default class TimeSeriesColumnControl extends React.Component {
               placeholder="Date format string"
             />,
           )}
+        <ButtonBar>
+          <Button buttonSize="small" onClick={this.onClose} cta>
+            {t('Close')}
+          </Button>
+          <Button
+            buttonStyle="primary"
+            buttonSize="small"
+            onClick={this.onSave}
+            cta
+          >
+            {t('Save')}
+          </Button>
+        </ButtonBar>
       </div>
     );
   }
@@ -310,6 +358,8 @@ export default class TimeSeriesColumnControl extends React.Component {
           placement="right"
           content={this.renderPopover()}
           title="Column Configuration"
+          visible={this.state.popoverVisible}
+          onVisibleChange={this.onPopoverVisibleChange}
         >
           <InfoTooltipWithTrigger
             icon="edit"