This is an automated email from the ASF dual-hosted git repository.
yjc pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push:
new b2636f0 fix: Explore popovers issues (#11428)
b2636f0 is described below
commit b2636f01bbdb3a20fff85907571cd1b291bc856b
Author: Kamil Gabryjelski <[email protected]>
AuthorDate: Wed Oct 28 17:38:52 2020 +0100
fix: Explore popovers issues (#11428)
* Fix spaces and comas not working in filter popover
* Fix popup not opening automatically
* Add e2e test
* Remove only from test
* Remove redundant test, add checking label content
* Add comments to e2e test
* Fix unit test
* Use destructuring
* Always open popup for functions and saved metrics, too
* Fix popover for adhoc metrics too
* Small refactor to consistency
* Refactor for consistency
* Remove redundant functions
* Test fix
Co-authored-by: Jesse Yang <[email protected]>
---
.../integration/explore/AdhocFilters.test.ts | 47 +++++++-----
.../integration/explore/AdhocMetrics.test.ts | 1 -
.../explore/components/AdhocFilterOption_spec.jsx | 4 +-
.../explore/components/AdhocMetricOption_spec.jsx | 12 +--
.../src/common/components/Popover.tsx | 3 +-
.../src/explore/components/AdhocFilterOption.jsx | 85 +++++++++++-----------
.../src/explore/components/AdhocMetricOption.jsx | 72 ++++++++----------
.../components/controls/AdhocFilterControl.jsx | 6 +-
8 files changed, 119 insertions(+), 111 deletions(-)
diff --git
a/superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
b/superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
index 6522555..9b7c5ae 100644
---
a/superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
+++
b/superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
@@ -50,13 +50,17 @@ describe('AdhocFilters', () => {
});
it('Set simple adhoc filter', () => {
- cy.get('#filter-edit-popover').within(() => {
- cy.get('[data-test=adhoc-filter-simple-value]').within(() => {
- cy.get('.Select__control').click();
- cy.get('input[type=text]').focus().type('Any{enter}');
- });
- cy.get('button').contains('Save').click();
- });
+ cy.get('[data-test=adhoc-filter-simple-value] .Select__control').click();
+ cy.get('[data-test=adhoc-filter-simple-value] input[type=text]')
+ .focus()
+ .type('Jack{enter}', { delay: 20 });
+
+ cy.get('[data-test="adhoc-filter-edit-popover-save-button"]').click();
+
+ cy.get(
+ '[data-test=adhoc_filters] .Select__control span.option-label',
+ ).contains('name = Jack');
+
cy.get('button[data-test="run-query-button"]').click();
cy.verifySliceSuccess({
waitAlias: '@postJson',
@@ -65,26 +69,35 @@ describe('AdhocFilters', () => {
});
it('Set custom adhoc filter', () => {
- cy.visitChartByName('Num Births Trend');
- cy.verifySliceSuccess({ waitAlias: '@postJson' });
+ const filterType = 'name';
+ const filterContent = "'Amy' OR name = 'Donald'";
cy.get('[data-test=adhoc_filters] .Select__control')
.scrollIntoView()
.click();
+
+ // remove previous input
cy.get('[data-test=adhoc_filters] input[type=text]')
.focus()
- .type('name{enter}', { delay: 20 });
- cy.get('[data-test="adhoc_filters"]').within(() => {
- cy.contains('name = ').should('be.visible').click();
- });
+ .type('{backspace}');
+
+ cy.get('[data-test=adhoc_filters] input[type=text]')
+ .focus()
+ .type(`${filterType}{enter}`);
+
cy.wait('@filterValues');
+ // selecting a new filter should auto-open the popup,
+ // so the tabshould be visible by now
cy.get('#filter-edit-popover #adhoc-filter-edit-tabs-tab-SQL').click();
cy.get('#filter-edit-popover .ace_content').click();
- cy.get('#filter-edit-popover .ace_text-input').type(
- "'Amy' OR name = 'Bob'",
- );
- cy.get('#filter-edit-popover button').contains('Save').click();
+ cy.get('#filter-edit-popover .ace_text-input').type(filterContent);
+ cy.get('[data-test="adhoc-filter-edit-popover-save-button"]').click();
+
+ // check if the filter was saved correctly
+ cy.get(
+ '[data-test=adhoc_filters] .Select__control span.option-label',
+ ).contains(`${filterType} = ${filterContent}`);
cy.get('button[data-test="run-query-button"]').click();
cy.verifySliceSuccess({
diff --git
a/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
b/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
index 85db968..b422673 100644
---
a/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
+++
b/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
@@ -42,7 +42,6 @@ describe('AdhocMetrics', () => {
.trigger('mousedown')
.click();
- cy.get('[data-test="option-label"]').first().click();
cy.get('[data-test="AdhocMetricEditTitle#trigger"]').click();
cy.get('[data-test="AdhocMetricEditTitle#input"]').type(metricName);
cy.get('[data-test="AdhocMetricEdit#save"]').contains('Save').click();
diff --git
a/superset-frontend/spec/javascripts/explore/components/AdhocFilterOption_spec.jsx
b/superset-frontend/spec/javascripts/explore/components/AdhocFilterOption_spec.jsx
index efdd7c1..1c47af8 100644
---
a/superset-frontend/spec/javascripts/explore/components/AdhocFilterOption_spec.jsx
+++
b/superset-frontend/spec/javascripts/explore/components/AdhocFilterOption_spec.jsx
@@ -19,7 +19,7 @@
/* eslint-disable no-unused-expressions */
import React from 'react';
import sinon from 'sinon';
-import { styledShallow as shallow } from 'spec/helpers/theming';
+import { shallow } from 'enzyme';
import Popover from 'src/common/components/Popover';
import Label from 'src/components/Label';
@@ -46,7 +46,7 @@ function setup(overrides) {
datasource: {},
...overrides,
};
- const wrapper = shallow(<AdhocFilterOption {...props} />).dive();
+ const wrapper = shallow(<AdhocFilterOption {...props} />);
return { wrapper };
}
diff --git
a/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx
b/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx
index c2e6629..9d358a0 100644
---
a/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx
+++
b/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx
@@ -19,7 +19,7 @@
/* eslint-disable no-unused-expressions */
import React from 'react';
import sinon from 'sinon';
-import { styledShallow as shallow } from 'spec/helpers/theming';
+import { shallow } from 'enzyme';
import Popover from 'src/common/components/Popover';
import Label from 'src/components/Label';
@@ -46,7 +46,7 @@ function setup(overrides) {
columns,
...overrides,
};
- const wrapper = shallow(<AdhocMetricOption {...props} />).dive();
+ const wrapper = shallow(<AdhocMetricOption {...props} />);
return { wrapper, onMetricEdit };
}
@@ -73,11 +73,13 @@ describe('AdhocMetricOption', () => {
it('returns to default labels when the custom label is cleared', () => {
const { wrapper } = setup();
+ expect(wrapper.state('title').label).toBe('SUM(value)');
+
wrapper.instance().onLabelChange({ target: { value: 'new label' } });
+ expect(wrapper.state('title').label).toBe('new label');
+
wrapper.instance().onLabelChange({ target: { value: '' } });
- // close and open the popover
- wrapper.instance().closeMetricEditOverlay();
- wrapper.instance().onOverlayEntered();
+
expect(wrapper.state('title').label).toBe('SUM(value)');
expect(wrapper.state('title').hasCustomLabel).toBe(false);
});
diff --git a/superset-frontend/src/common/components/Popover.tsx
b/superset-frontend/src/common/components/Popover.tsx
index 1f8442c..b2682c4 100644
--- a/superset-frontend/src/common/components/Popover.tsx
+++ b/superset-frontend/src/common/components/Popover.tsx
@@ -17,8 +17,7 @@
* under the License.
*/
import { Popover as AntdPopover } from 'src/common/components';
-import { styled } from '@superset-ui/core';
-const SupersetPopover = styled(AntdPopover)``;
+const SupersetPopover = AntdPopover;
export default SupersetPopover;
diff --git a/superset-frontend/src/explore/components/AdhocFilterOption.jsx
b/superset-frontend/src/explore/components/AdhocFilterOption.jsx
index 29240e4..42ba81d 100644
--- a/superset-frontend/src/explore/components/AdhocFilterOption.jsx
+++ b/superset-frontend/src/explore/components/AdhocFilterOption.jsx
@@ -18,10 +18,10 @@
*/
import React from 'react';
import PropTypes from 'prop-types';
-import Popover from 'src/common/components/Popover';
-import { t, withTheme } from '@superset-ui/core';
+import { t } from '@superset-ui/core';
import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
+import Popover from 'src/common/components/Popover';
import Label from 'src/components/Label';
import AdhocFilterEditPopover from './AdhocFilterEditPopover';
import AdhocFilter from '../AdhocFilter';
@@ -44,57 +44,63 @@ const propTypes = {
class AdhocFilterOption extends React.PureComponent {
constructor(props) {
super(props);
- this.closeFilterEditOverlay = this.closeFilterEditOverlay.bind(this);
this.onPopoverResize = this.onPopoverResize.bind(this);
- this.onOverlayEntered = this.onOverlayEntered.bind(this);
- this.onOverlayExited = this.onOverlayExited.bind(this);
- this.handleVisibleChange = this.handleVisibleChange.bind(this);
- this.state = { overlayShown: false };
+ this.closePopover = this.closePopover.bind(this);
+ this.togglePopover = this.togglePopover.bind(this);
+ this.state = {
+ // automatically open the popover the the metric is new
+ popoverVisible: !!props.adhocFilter.isNew,
+ };
}
- onPopoverResize() {
- this.forceUpdate();
- }
-
- onOverlayEntered() {
- // isNew is used to indicate whether to automatically open the overlay
- // once the overlay has been opened, the metric/filter will never be
- // considered new again.
- this.props.adhocFilter.isNew = false;
- this.setState({ overlayShown: true });
+ componentDidMount() {
+ const { adhocFilter } = this.props;
+ // isNew is used to auto-open the popup. Once popup is opened, it's not
+ // considered new anymore.
+ // put behind setTimeout so in case consequetive re-renderings are
triggered
+ // for some reason, the popup can still show up.
+ setTimeout(() => {
+ adhocFilter.isNew = false;
+ });
}
- onOverlayExited() {
- this.setState({ overlayShown: false });
+ onPopoverResize() {
+ this.forceUpdate();
}
- closeFilterEditOverlay() {
- this.setState({ overlayShown: false });
+ closePopover() {
+ this.setState({ popoverVisible: false });
}
- handleVisibleChange(visible) {
- if (visible) {
- this.onOverlayEntered();
- } else {
- this.onOverlayExited();
- }
+ togglePopover(visible) {
+ this.setState(({ popoverVisible }) => {
+ return {
+ popoverVisible: visible === undefined ? !popoverVisible : visible,
+ };
+ });
}
render() {
const { adhocFilter } = this.props;
- const content = (
+ const overlayContent = (
<AdhocFilterEditPopover
- onResize={this.onPopoverResize}
adhocFilter={adhocFilter}
- onChange={this.props.onFilterEdit}
- onClose={this.closeFilterEditOverlay}
options={this.props.options}
datasource={this.props.datasource}
partitionColumn={this.props.partitionColumn}
+ onResize={this.onPopoverResize}
+ onClose={this.closePopover}
+ onChange={this.props.onFilterEdit}
/>
);
+
return (
- <div role="button" tabIndex={0} onMouseDown={e => e.stopPropagation()}>
+ <div
+ role="button"
+ tabIndex={0}
+ onMouseDown={e => e.stopPropagation()}
+ onKeyDown={e => e.stopPropagation()}
+ >
{adhocFilter.isExtra && (
<InfoTooltipWithTrigger
icon="exclamation-triangle"
@@ -109,19 +115,14 @@ class AdhocFilterOption extends React.PureComponent {
<Popover
placement="right"
trigger="click"
- disabled
- content={content}
+ content={overlayContent}
defaultVisible={adhocFilter.isNew}
- visible={this.state.overlayShown}
- onVisibleChange={this.handleVisibleChange}
+ visible={this.state.popoverVisible}
+ onVisibleChange={this.togglePopover}
>
<Label className="option-label adhoc-option adhoc-filter-option">
{adhocFilter.getDefaultLabel()}
- <i
- className={`fa fa-caret-${
- this.state.overlayShown ? 'left' : 'right'
- } adhoc-label-arrow`}
- />
+ <i className="fa fa-caret-right adhoc-label-arrow" />
</Label>
</Popover>
</div>
@@ -129,6 +130,6 @@ class AdhocFilterOption extends React.PureComponent {
}
}
-export default withTheme(AdhocFilterOption);
+export default AdhocFilterOption;
AdhocFilterOption.propTypes = propTypes;
diff --git a/superset-frontend/src/explore/components/AdhocMetricOption.jsx
b/superset-frontend/src/explore/components/AdhocMetricOption.jsx
index 4c47e22..d030e67 100644
--- a/superset-frontend/src/explore/components/AdhocMetricOption.jsx
+++ b/superset-frontend/src/explore/components/AdhocMetricOption.jsx
@@ -18,7 +18,6 @@
*/
import React from 'react';
import PropTypes from 'prop-types';
-import { withTheme } from '@superset-ui/core';
import Popover from 'src/common/components/Popover';
import Label from 'src/components/Label';
@@ -38,13 +37,12 @@ const propTypes = {
class AdhocMetricOption extends React.PureComponent {
constructor(props) {
super(props);
- this.closeMetricEditOverlay = this.closeMetricEditOverlay.bind(this);
- this.onOverlayEntered = this.onOverlayEntered.bind(this);
this.onPopoverResize = this.onPopoverResize.bind(this);
- this.handleVisibleChange = this.handleVisibleChange.bind(this);
this.onLabelChange = this.onLabelChange.bind(this);
+ this.closePopover = this.closePopover.bind(this);
+ this.togglePopover = this.togglePopover.bind(this);
this.state = {
- overlayShown: false,
+ popoverVisible: undefined,
title: {
label: props.adhocMetric.label,
hasCustomLabel: props.adhocMetric.hasCustomLabel,
@@ -52,12 +50,23 @@ class AdhocMetricOption extends React.PureComponent {
};
}
+ componentDidMount() {
+ const { adhocMetric } = this.props;
+ // isNew is used to auto-open the popup. Once popup is opened, it's not
+ // considered new anymore.
+ // put behind setTimeout so in case consequetive re-renderings are
triggered
+ // for some reason, the popup can still show up.
+ setTimeout(() => {
+ adhocMetric.isNew = false;
+ });
+ }
+
onLabelChange(e) {
const label = e.target.value;
this.setState({
title: {
- label,
- hasCustomLabel: true,
+ label: label || this.props.adhocMetric.label,
+ hasCustomLabel: !!label,
},
});
}
@@ -66,43 +75,30 @@ class AdhocMetricOption extends React.PureComponent {
this.forceUpdate();
}
- onOverlayEntered() {
- // isNew is used to indicate whether to automatically open the overlay
- // once the overlay has been opened, the metric/filter will never be
- // considered new again.
- this.props.adhocMetric.isNew = false;
- this.setState({
- overlayShown: true,
- title: {
- label: this.props.adhocMetric.label,
- hasCustomLabel: this.props.adhocMetric.hasCustomLabel,
- },
- });
- }
-
- closeMetricEditOverlay() {
- this.setState({ overlayShown: false });
+ closePopover() {
+ this.setState({ popoverVisible: false });
}
- handleVisibleChange(visible) {
- if (visible) {
- this.onOverlayEntered();
- } else {
- this.closeMetricEditOverlay();
- }
+ togglePopover(visible) {
+ this.setState(({ popoverVisible }) => {
+ return {
+ popoverVisible: visible === undefined ? !popoverVisible : visible,
+ };
+ });
}
render() {
const { adhocMetric } = this.props;
+
const overlayContent = (
<AdhocMetricEditPopover
- onResize={this.onPopoverResize}
adhocMetric={adhocMetric}
title={this.state.title}
- onChange={this.props.onMetricEdit}
- onClose={this.closeMetricEditOverlay}
columns={this.props.columns}
datasourceType={this.props.datasourceType}
+ onResize={this.onPopoverResize}
+ onClose={this.closePopover}
+ onChange={this.props.onMetricEdit}
/>
);
@@ -129,17 +125,13 @@ class AdhocMetricOption extends React.PureComponent {
disabled
content={overlayContent}
defaultVisible={adhocMetric.isNew}
- onVisibleChange={this.handleVisibleChange}
- visible={this.state.overlayShown}
+ visible={this.state.popoverVisible}
+ onVisibleChange={this.togglePopover}
title={popoverTitle}
>
<Label className="option-label adhoc-option"
data-test="option-label">
{adhocMetric.label}
- <i
- className={`fa fa-caret-${
- this.state.overlayShown ? 'left' : 'right'
- } adhoc-label-arrow`}
- />
+ <i className="fa fa-caret-right adhoc-label-arrow" />
</Label>
</Popover>
</div>
@@ -147,6 +139,6 @@ class AdhocMetricOption extends React.PureComponent {
}
}
-export default withTheme(AdhocMetricOption);
+export default AdhocMetricOption;
AdhocMetricOption.propTypes = propTypes;
diff --git
a/superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx
b/superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx
index 13f8d87..28e6b42 100644
--- a/superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx
+++ b/superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx
@@ -180,9 +180,10 @@ export default class AdhocFilterControl extends
React.Component {
operator: OPERATORS['>'],
comparator: 0,
clause: CLAUSES.HAVING,
+ isNew: true,
});
}
- // has a custom label
+ // has a custom label, meaning it's custom column
if (option.label) {
return new AdhocFilter({
expressionType:
@@ -196,6 +197,7 @@ export default class AdhocFilterControl extends
React.Component {
operator: OPERATORS['>'],
comparator: 0,
clause: CLAUSES.HAVING,
+ isNew: true,
});
}
// add a new filter item
@@ -262,7 +264,7 @@ export default class AdhocFilterControl extends
React.Component {
render() {
return (
- <div className="metrics-select">
+ <div className="metrics-select" data-test="adhoc-filter-control">
<ControlHeader {...this.props} />
<OnPasteSelect
isMulti
