From 09555b5f9a70e8670d41f641c210b21adf5f0567 Mon Sep 17 00:00:00 2001 From: Diego Medina Date: Fri, 29 Apr 2022 11:33:05 -0300 Subject: [PATCH 1/2] feature: When editing the label/title in the Metrics popover, hitting Enter should save what you've typed --- .../src/assets/stylesheets/superset.less | 7 +- .../AdhocMetricEditPopoverTitle.jsx | 115 -------------- .../AdhocMetricEditPopoverTitle.test.jsx | 70 --------- .../AdhocMetricEditPopoverTitle.test.tsx | 141 ++++++++++++++++++ .../AdhocMetricEditPopoverTitle.tsx | 119 +++++++++++++++ 5 files changed, 266 insertions(+), 186 deletions(-) delete mode 100644 superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.jsx delete mode 100644 superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.test.jsx create mode 100644 superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.test.tsx create mode 100644 superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx diff --git a/superset-frontend/src/assets/stylesheets/superset.less b/superset-frontend/src/assets/stylesheets/superset.less index 0cf419b30d190..d0dd85d8ef39b 100644 --- a/superset-frontend/src/assets/stylesheets/superset.less +++ b/superset-frontend/src/assets/stylesheets/superset.less @@ -518,9 +518,14 @@ tr.reactable-column-header th.reactable-header-sortable { padding-right: 17px; } +.metric-edit-popover-label { + display: inline-block; + padding: 2px 0; +} + .metric-edit-popover-label-input { border-radius: @border-radius-large; - height: 30px; + height: 26px; padding-left: 10px; } diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.jsx deleted file mode 100644 index cef62505e4d0e..0000000000000 --- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.jsx +++ /dev/null @@ -1,115 +0,0 @@ -/** - * 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 React from 'react'; -import { t } from '@superset-ui/core'; -import PropTypes from 'prop-types'; -import { Input } from 'src/components/Input'; -import { Tooltip } from 'src/components/Tooltip'; - -const propTypes = { - title: PropTypes.shape({ - label: PropTypes.string, - hasCustomLabel: PropTypes.bool, - }), - onChange: PropTypes.func.isRequired, - isEditDisabled: PropTypes.bool, -}; - -export default class AdhocMetricEditPopoverTitle extends React.Component { - constructor(props) { - super(props); - this.onMouseOver = this.onMouseOver.bind(this); - this.onMouseOut = this.onMouseOut.bind(this); - this.onClick = this.onClick.bind(this); - this.onBlur = this.onBlur.bind(this); - this.onInputBlur = this.onInputBlur.bind(this); - this.state = { - isHovered: false, - isEditMode: false, - }; - } - - onMouseOver() { - this.setState({ isHovered: true }); - } - - onMouseOut() { - this.setState({ isHovered: false }); - } - - onClick() { - this.setState({ isEditMode: true }); - } - - onBlur() { - this.setState({ isEditMode: false }); - } - - onInputBlur(e) { - if (e.target.value === '') { - this.props.onChange(e); - } - this.onBlur(); - } - - render() { - const { title, onChange, isEditDisabled } = this.props; - const defaultLabel = t('My metric'); - - if (isEditDisabled) { - return ( - {title.label || defaultLabel} - ); - } - - return this.state.isEditMode ? ( - - ) : ( - - - {title.label || defaultLabel} -   - - - - ); - } -} -AdhocMetricEditPopoverTitle.propTypes = propTypes; diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.test.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.test.jsx deleted file mode 100644 index dd2b007bf91be..0000000000000 --- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.test.jsx +++ /dev/null @@ -1,70 +0,0 @@ -/** - * 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. - */ -/* eslint-disable no-unused-expressions */ -import React from 'react'; -import sinon from 'sinon'; -import { shallow } from 'enzyme'; -import { Tooltip } from 'src/components/Tooltip'; - -import AdhocMetricEditPopoverTitle from 'src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle'; - -const title = { - label: 'Title', - hasCustomLabel: false, -}; - -function setup(overrides) { - const onChange = sinon.spy(); - const props = { - title, - onChange, - ...overrides, - }; - const wrapper = shallow(); - return { wrapper, onChange }; -} - -describe('AdhocMetricEditPopoverTitle', () => { - it('renders an OverlayTrigger wrapper with the title', () => { - const { wrapper } = setup(); - expect(wrapper.find(Tooltip)).toExist(); - expect( - wrapper.find('[data-test="AdhocMetricEditTitle#trigger"]').text(), - ).toBe(`${title.label}\xa0`); - }); - - it('transfers to edit mode when clicked', () => { - const { wrapper } = setup(); - expect(wrapper.state('isEditMode')).toBe(false); - wrapper - .find('[data-test="AdhocMetricEditTitle#trigger"]') - .simulate('click'); - expect(wrapper.state('isEditMode')).toBe(true); - }); - - it('Render non-interactive span with title when edit is disabled', () => { - const { wrapper } = setup({ isEditDisabled: true }); - expect( - wrapper.find('[data-test="AdhocMetricTitle"]').exists(), - ).toBeTruthy(); - expect( - wrapper.find('[data-test="AdhocMetricEditTitle#trigger"]').exists(), - ).toBeFalsy(); - }); -}); diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.test.tsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.test.tsx new file mode 100644 index 0000000000000..a91e0cac6f5af --- /dev/null +++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.test.tsx @@ -0,0 +1,141 @@ +/** + * 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 React from 'react'; +import userEvent from '@testing-library/user-event'; +import { + screen, + render, + fireEvent, + waitFor, +} from 'spec/helpers/testing-library'; + +import AdhocMetricEditPopoverTitle, { + AdhocMetricEditPopoverTitleProps, +} from 'src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle'; + +const titleProps = { + label: 'COUNT(*)', + hasCustomLabel: false, +}; + +const setup = (props: Partial = {}) => { + const onChange = jest.fn(); + + const { container } = render( + , + ); + + return { container, onChange }; +}; + +test('should render', async () => { + const { container } = setup(); + expect(container).toBeInTheDocument(); + + expect(screen.queryByTestId('AdhocMetricTitle')).not.toBeInTheDocument(); + expect(screen.getByText(titleProps.label)).toBeVisible(); +}); + +test('should render tooltip on hover', async () => { + const { container } = setup(); + + expect(screen.queryByText('Click to edit label')).not.toBeInTheDocument(); + fireEvent.mouseOver(screen.getByTestId('AdhocMetricEditTitle#trigger')); + + expect(await screen.findByText('Click to edit label')).toBeInTheDocument(); + expect( + container.parentElement?.getElementsByClassName('ant-tooltip-hidden') + .length, + ).toBe(0); + + fireEvent.mouseOut(screen.getByTestId('AdhocMetricEditTitle#trigger')); + await waitFor(() => { + expect( + container.parentElement?.getElementsByClassName('ant-tooltip-hidden') + .length, + ).toBe(1); + }); +}); + +test('render non-interactive span with title when edit is disabled', async () => { + const { container } = setup({ isEditDisabled: true }); + expect(container).toBeInTheDocument(); + + expect(screen.queryByTestId('AdhocMetricTitle')).toBeInTheDocument(); + expect(screen.getByText(titleProps.label)).toBeVisible(); + expect( + screen.queryByTestId('AdhocMetricEditTitle#trigger'), + ).not.toBeInTheDocument(); +}); + +test('render default label if no title is provided', async () => { + const { container } = setup({ title: undefined }); + expect(container).toBeInTheDocument(); + + expect(screen.queryByTestId('AdhocMetricTitle')).not.toBeInTheDocument(); + expect(screen.getByText('My metric')).toBeVisible(); +}); + +test('start and end the title edit mode', async () => { + const { container, onChange } = setup(); + expect(container).toBeInTheDocument(); + + expect(container.getElementsByTagName('i')[0]).toBeVisible(); + expect(screen.getByText(titleProps.label)).toBeVisible(); + expect( + screen.queryByTestId('AdhocMetricEditTitle#input'), + ).not.toBeInTheDocument(); + + fireEvent.click( + container.getElementsByClassName('AdhocMetricEditPopoverTitle')[0], + ); + + expect(await screen.findByTestId('AdhocMetricEditTitle#input')).toBeVisible(); + userEvent.type(screen.getByTestId('AdhocMetricEditTitle#input'), 'Test'); + + expect(onChange).toHaveBeenCalledTimes(4); + fireEvent.keyPress(screen.getByTestId('AdhocMetricEditTitle#input'), { + key: 'Enter', + charCode: 13, + }); + + expect( + screen.queryByTestId('AdhocMetricEditTitle#input'), + ).not.toBeInTheDocument(); + + fireEvent.click( + container.getElementsByClassName('AdhocMetricEditPopoverTitle')[0], + ); + + expect(await screen.findByTestId('AdhocMetricEditTitle#input')).toBeVisible(); + userEvent.type( + screen.getByTestId('AdhocMetricEditTitle#input'), + 'Second test', + ); + expect(onChange).toHaveBeenCalled(); + + fireEvent.blur(screen.getByTestId('AdhocMetricEditTitle#input')); + expect( + screen.queryByTestId('AdhocMetricEditTitle#input'), + ).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx new file mode 100644 index 0000000000000..21c196e4c2369 --- /dev/null +++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx @@ -0,0 +1,119 @@ +/** + * 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 React, { + ChangeEventHandler, + FocusEvent, + KeyboardEvent, + useCallback, + useState, +} from 'react'; +import { t } from '@superset-ui/core'; +import { Input } from 'src/components/Input'; +import { Tooltip } from 'src/components/Tooltip'; + +export interface AdhocMetricEditPopoverTitleProps { + title?: { + label?: string; + hasCustomLabel?: boolean; + }; + isEditDisabled?: boolean; + onChange: ChangeEventHandler; +} + +const AdhocMetricEditPopoverTitle: React.FC = + ({ title, isEditDisabled, onChange }) => { + const [isHovered, setIsHovered] = useState(false); + const [isEditMode, setIsEditMode] = useState(false); + + const defaultLabel = t('My metric'); + + const handleMouseOver = useCallback(() => setIsHovered(true), []); + const handleMouseOut = useCallback(() => setIsHovered(false), []); + const handleClick = useCallback(() => setIsEditMode(true), []); + const handleBlur = useCallback(() => setIsEditMode(false), []); + + const handleKeyPress = useCallback( + (ev: KeyboardEvent) => { + if (ev.key === 'Enter') { + ev.preventDefault(); + handleBlur(); + } + }, + [handleBlur], + ); + + const handleInputBlur = useCallback( + (e: FocusEvent) => { + if (e.target.value === '') { + onChange(e); + } + + handleBlur(); + }, + [onChange, handleBlur], + ); + + if (isEditDisabled) { + return ( + {title?.label || defaultLabel} + ); + } + + if (isEditMode) { + return ( + + ); + } + + return ( + + + + {title?.label || defaultLabel} + +   + + + + ); + }; + +export default AdhocMetricEditPopoverTitle; From 110e39a59e7cd160a44cb957e660a126885ba2f6 Mon Sep 17 00:00:00 2001 From: Diego Medina Date: Fri, 3 Jun 2022 16:00:46 -0300 Subject: [PATCH 2/2] Apply emotion templating to input/input labels --- .../src/assets/stylesheets/superset.less | 11 ---------- .../DndColumnSelectPopoverTitle.jsx | 11 +++++++--- .../AdhocMetricEditPopoverTitle.tsx | 20 +++++++++++++------ 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/superset-frontend/src/assets/stylesheets/superset.less b/superset-frontend/src/assets/stylesheets/superset.less index d0dd85d8ef39b..5808d0144bc73 100644 --- a/superset-frontend/src/assets/stylesheets/superset.less +++ b/superset-frontend/src/assets/stylesheets/superset.less @@ -518,17 +518,6 @@ tr.reactable-column-header th.reactable-header-sortable { padding-right: 17px; } -.metric-edit-popover-label { - display: inline-block; - padding: 2px 0; -} - -.metric-edit-popover-label-input { - border-radius: @border-radius-large; - height: 26px; - padding-left: 10px; -} - .align-right { text-align: right; } diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectPopoverTitle.jsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectPopoverTitle.jsx index eecce0b33335b..b50abb9aae6ee 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectPopoverTitle.jsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectPopoverTitle.jsx @@ -17,10 +17,16 @@ * under the License. */ import React, { useCallback, useState } from 'react'; -import { t } from '@superset-ui/core'; +import { t, styled } from '@superset-ui/core'; import { Input } from 'src/components/Input'; import { Tooltip } from 'src/components/Tooltip'; +const StyledInput = styled(Input)` + border-radius: ${({ theme }) => theme.borderRadius}; + height: 26px; + padding-left: ${({ theme }) => theme.gridUnit * 2.5}px; +`; + export const DndColumnSelectPopoverTitle = ({ title, onChange, @@ -63,8 +69,7 @@ export const DndColumnSelectPopoverTitle = ({ } return isEditMode ? ( - theme.borderRadius}; + height: 26px; + padding-left: ${({ theme }) => theme.gridUnit * 2.5}px; +`; + export interface AdhocMetricEditPopoverTitleProps { title?: { label?: string; @@ -77,8 +88,7 @@ const AdhocMetricEditPopoverTitle: React.FC = if (isEditMode) { return ( - = role="button" tabIndex={0} > - - {title?.label || defaultLabel} - + {title?.label || defaultLabel}