diff --git a/superset-frontend/spec/javascripts/explore/components/BoundsControl_spec.jsx b/superset-frontend/spec/javascripts/explore/components/BoundsControl_spec.jsx index 279ec404a85..2e25d130f02 100644 --- a/superset-frontend/spec/javascripts/explore/components/BoundsControl_spec.jsx +++ b/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(); - }); - - 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(); + expect(screen.getAllByRole('spinbutton')).toHaveLength(2); +}); + +test('receives null on non-numeric', async () => { + render(); + 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(); + 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]), + ); }); diff --git a/superset-frontend/src/common/components/index.tsx b/superset-frontend/src/common/components/index.tsx index 1706d11befe..4e09eeb9c44 100644 --- a/superset-frontend/src/common/components/index.tsx +++ b/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; diff --git a/superset-frontend/src/explore/components/controls/BoundsControl.jsx b/superset-frontend/src/explore/components/controls/BoundsControl.jsx index da9eda2590d..ff22cebf3b1 100644 --- a/superset-frontend/src/explore/components/controls/BoundsControl.jsx +++ b/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 (
- - - - - - - - + + + +
); } diff --git a/superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/TimeSeriesColumnControl.test.tsx b/superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/TimeSeriesColumnControl.test.tsx index 1173101a3b8..4383075ded9 100644 --- a/superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/TimeSeriesColumnControl.test.tsx +++ b/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(); 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(); - 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(); 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(); + 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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 }), + ); }); diff --git a/superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.jsx b/superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.jsx index ab3583a9288..aef39a4a4c8 100644 --- a/superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.jsx +++ b/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 ( - + {label} - + {control} - + ); } renderPopover() { return ( -
+
{this.formRow( 'Label', 'The column header label', @@ -297,6 +332,19 @@ export default class TimeSeriesColumnControl extends React.Component { placeholder="Date format string" />, )} + + + +
); } @@ -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} >