Skip to content

Commit

Permalink
fix: Issues with filters and metrics popovers (apache#11578)
Browse files Browse the repository at this point in the history 
* Fix bugs in AdhocFilterEditPopover

* Fix bugs in AdhocMetricEditPopover

* Remove handleMultiComparatorInputHeightChange function

* Fix tests
  • Loading branch information
@kgabryje @auxten
kgabryje authored and auxten committed Nov 20, 2020
1 parent 0354c00 commit 0893b97
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 72 deletions.
12 changes: 0 additions & 12 deletions ...ntend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,16 +199,4 @@ describe('AdhocFilterEditPopoverSimpleTabContent', () => {
}),
);
});

it('expands when its multi comparator input field expands', () => {
const { wrapper, onHeightChange } = setup();

wrapper.instance().multiComparatorComponent = {
select: { select: { controlRef: { clientHeight: 57 } } },
};
wrapper.instance().handleMultiComparatorInputHeightChange();

expect(onHeightChange.calledOnce).toBe(true);
expect(onHeightChange.lastCall.args[0]).toBe(27);
});
});
4 changes: 2 additions & 2 deletions superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopover_spec.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,9 @@ describe('AdhocFilterEditPopover', () => {
wrapper.instance().onDragDown = sinon.spy();
wrapper.instance().forceUpdate();

expect(wrapper.find('i.fa-expand')).toExist();
expect(wrapper.find('.fa-expand')).toExist();
expect(wrapper.instance().onDragDown.calledOnce).toBe(false);
wrapper.find('i.fa-expand').simulate('mouseDown', {});
wrapper.find('.fa-expand').simulate('mouseDown', {});
expect(wrapper.instance().onDragDown.calledOnce).toBe(true);
});
});
4 changes: 2 additions & 2 deletions superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ describe('AdhocMetricEditPopover', () => {
wrapper.instance().onDragDown = sinon.spy();
wrapper.instance().forceUpdate();

expect(wrapper.find('i.fa-expand')).toExist();
expect(wrapper.find('.fa-expand')).toExist();
expect(wrapper.instance().onDragDown.calledOnce).toBe(false);
wrapper.find('i.fa-expand').simulate('mouseDown');
wrapper.find('.fa-expand').simulate('mouseDown');
expect(wrapper.instance().onDragDown.calledOnce).toBe(true);
});
});
14 changes: 11 additions & 3 deletions superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import React from 'react';
import PropTypes from 'prop-types';
import Button from 'src/components/Button';
import { t } from '@superset-ui/core';
import { styled, t } from '@superset-ui/core';

import Tabs from 'src/common/components/Tabs';
import columnType from '../propTypes/columnType';
Expand All @@ -45,7 +45,11 @@ const propTypes = {
theme: PropTypes.object,
};

const startingWidth = 300;
const ResizeIcon = styled.i`
margin-left: ${({ theme }) => theme.gridUnit * 2}px;
`;

const startingWidth = 320;
const startingHeight = 240;

export default class AdhocFilterEditPopover extends React.Component {
Expand All @@ -63,6 +67,8 @@ export default class AdhocFilterEditPopover extends React.Component {
width: startingWidth,
height: startingHeight,
};

this.popoverContentRef = React.createRef();
}

componentDidMount() {
Expand Down Expand Up @@ -136,6 +142,7 @@ export default class AdhocFilterEditPopover extends React.Component {
id="filter-edit-popover"
{...popoverProps}
data-test="filter-edit-popover"
ref={this.popoverContentRef}
>
<Tabs
id="adhoc-filter-edit-tabs"
Expand All @@ -156,6 +163,7 @@ export default class AdhocFilterEditPopover extends React.Component {
datasource={datasource}
onHeightChange={this.adjustHeight}
partitionColumn={partitionColumn}
popoverRef={this.popoverContentRef}
/>
</Tabs.TabPane>
<Tabs.TabPane
Expand Down Expand Up @@ -195,7 +203,7 @@ export default class AdhocFilterEditPopover extends React.Component {
>
{t('Save')}
</Button>
<i
<ResizeIcon
role="button"
aria-label="Resize"
tabIndex={0}
Expand Down
51 changes: 12 additions & 39 deletions superset-frontend/src/explore/components/AdhocFilterEditPopoverSimpleTabContent.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const propTypes = {
onHeightChange: PropTypes.func.isRequired,
datasource: PropTypes.object,
partitionColumn: PropTypes.string,
popoverRef: PropTypes.object,
};

const defaultProps = {
Expand All @@ -73,8 +74,6 @@ function translateOperator(operator) {
return operator;
}

const SINGLE_LINE_SELECT_CONTROL_HEIGHT = 30;

export default class AdhocFilterEditPopoverSimpleTabContent extends React.Component {
constructor(props) {
super(props);
Expand All @@ -86,11 +85,9 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
this.refreshComparatorSuggestions = this.refreshComparatorSuggestions.bind(
this,
);
this.multiComparatorRef = this.multiComparatorRef.bind(this);

this.state = {
suggestions: [],
multiComparatorHeight: SINGLE_LINE_SELECT_CONTROL_HEIGHT,
abortActiveRequest: null,
};

Expand All @@ -101,21 +98,22 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
autosize: false,
clearable: false,
};

this.menuPortalProps = {
menuPortalTarget: props.popoverRef?.current || document.body,
menuPosition: 'fixed',
menuPlacement: 'bottom',
};
}

UNSAFE_componentWillMount() {
this.refreshComparatorSuggestions();
}

componentDidMount() {
this.handleMultiComparatorInputHeightChange();
}

componentDidUpdate(prevProps) {
if (prevProps.adhocFilter.subject !== this.props.adhocFilter.subject) {
this.refreshComparatorSuggestions();
}
this.handleMultiComparatorInputHeightChange();
}

onSubjectChange(option) {
Expand Down Expand Up @@ -192,27 +190,6 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
);
}

handleMultiComparatorInputHeightChange() {
if (this.multiComparatorComponent) {
const multiComparatorDOMNode = this.multiComparatorComponent?.select
?.select.controlRef;
if (multiComparatorDOMNode) {
if (
multiComparatorDOMNode.clientHeight !==
this.state.multiComparatorHeight
) {
this.props.onHeightChange(
multiComparatorDOMNode.clientHeight -
this.state.multiComparatorHeight,
);
this.setState({
multiComparatorHeight: multiComparatorDOMNode.clientHeight,
});
}
}
}
}

refreshComparatorSuggestions() {
const { datasource } = this.props;
const col = this.props.adhocFilter.subject;
Expand Down Expand Up @@ -270,12 +247,6 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
}
}

multiComparatorRef(ref) {
if (ref) {
this.multiComparatorComponent = ref;
}
}

renderSubjectOptionLabel(option) {
return <FilterDefinitionOption option={option} />;
}
Expand Down Expand Up @@ -328,17 +299,19 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
};

return (
<span>
<>
<FormGroup className="adhoc-filter-simple-column-dropdown">
<Select
{...this.selectProps}
{...this.menuPortalProps}
{...subjectSelectProps}
name="filter-column"
/>
</FormGroup>
<FormGroup>
<Select
{...this.selectProps}
{...this.menuPortalProps}
{...operatorSelectProps}
name="filter-operator"
/>
Expand All @@ -347,6 +320,7 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
{MULTI_OPERATORS.has(operator) ||
this.state.suggestions.length > 0 ? (
<SelectControl
{...this.menuPortalProps}
name="filter-value"
autoFocus
freeForm
Expand All @@ -357,7 +331,6 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
onChange={this.onComparatorChange}
showHeader={false}
noResultsText={t('type a value here')}
selectRef={this.multiComparatorRef}
disabled={DISABLE_INPUT_OPERATORS.includes(operator)}
/>
) : (
Expand All @@ -373,7 +346,7 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
/>
)}
</FormGroup>
</span>
</>
);
}
}
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export default class AdhocFilterEditPopoverSqlTabContent extends React.Component
<SQLEditor
ref={this.handleAceEditorRef}
keywords={keywords}
height={`${height - 100}px`}
height={`${height - 130}px`}
onChange={this.onSqlExpressionChange}
width="100%"
showGutter={false}
Expand Down
43 changes: 30 additions & 13 deletions superset-frontend/src/explore/components/AdhocMetricEditPopover.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { FormGroup } from 'react-bootstrap';
import Tabs from 'src/common/components/Tabs';
import Button from 'src/components/Button';
import Select from 'src/components/Select';
import { t } from '@superset-ui/core';
import { styled, t } from '@superset-ui/core';
import { ColumnOption } from '@superset-ui/chart-controls';

import FormLabel from 'src/components/FormLabel';
Expand Down Expand Up @@ -50,7 +50,11 @@ const defaultProps = {
columns: [],
};

const startingWidth = 300;
const ResizeIcon = styled.i`
margin-left: ${({ theme }) => theme.gridUnit * 2}px;
`;

const startingWidth = 320;
const startingHeight = 240;

export default class AdhocMetricEditPopover extends React.Component {
Expand All @@ -65,17 +69,28 @@ export default class AdhocMetricEditPopover extends React.Component {
this.onMouseUp = this.onMouseUp.bind(this);
this.handleAceEditorRef = this.handleAceEditorRef.bind(this);
this.refreshAceEditor = this.refreshAceEditor.bind(this);

this.popoverRef = React.createRef();

this.state = {
adhocMetric: this.props.adhocMetric,
width: startingWidth,
height: startingHeight,
};

this.selectProps = {
labelKey: 'label',
isMulti: false,
autosize: false,
clearable: true,
};

this.menuPortalProps = {
menuPosition: 'fixed',
menuPlacement: 'bottom',
menuPortalTarget: this.popoverRef.current,
};

document.addEventListener('mouseup', this.onMouseUp);
}

Expand Down Expand Up @@ -215,6 +230,7 @@ export default class AdhocMetricEditPopover extends React.Component {
<div
id="metrics-edit-popover"
data-test="metrics-edit-popover"
ref={this.popoverRef}
{...popoverProps}
>
<Tabs
Expand All @@ -237,6 +253,7 @@ export default class AdhocMetricEditPopover extends React.Component {
<Select
name="select-column"
{...this.selectProps}
{...this.menuPortalProps}
{...columnSelectProps}
/>
</FormGroup>
Expand All @@ -247,6 +264,7 @@ export default class AdhocMetricEditPopover extends React.Component {
<Select
name="select-aggregate"
{...this.selectProps}
{...this.menuPortalProps}
{...aggregateSelectProps}
autoFocus
/>
Expand All @@ -264,7 +282,7 @@ export default class AdhocMetricEditPopover extends React.Component {
showLoadingForImport
ref={this.handleAceEditorRef}
keywords={keywords}
height={`${this.state.height - 43}px`}
height={`${this.state.height - 80}px`}
onChange={this.onSqlExpressionChange}
width="100%"
showGutter={false}
Expand All @@ -285,28 +303,27 @@ export default class AdhocMetricEditPopover extends React.Component {
</Tabs.TabPane>
</Tabs>
<div>
<Button
buttonSize="small"
onClick={this.props.onClose}
data-test="AdhocMetricEdit#cancel"
cta
>
Close
</Button>
<Button
disabled={!stateIsValid}
buttonStyle={
hasUnsavedChanges && stateIsValid ? 'primary' : 'default'
}
buttonSize="small"
className="m-r-5"
data-test="AdhocMetricEdit#save"
onClick={this.onSave}
cta
>
Save
</Button>
<Button
buttonSize="small"
onClick={this.props.onClose}
data-test="AdhocMetricEdit#cancel"
cta
>
Close
</Button>
<i
<ResizeIcon
role="button"
aria-label="Resize"
tabIndex={0}
Expand Down
6 changes: 6 additions & 0 deletions superset-frontend/src/explore/components/controls/SelectControl.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ const propTypes = {
filterOption: PropTypes.func,
promptTextCreator: PropTypes.func,
commaChoosesOption: PropTypes.bool,
menuPortalTarget: PropTypes.element,
menuPosition: PropTypes.string,
menuPlacement: PropTypes.string,
};

const defaultProps = {
Expand Down Expand Up @@ -219,6 +222,9 @@ export default class SelectControl extends React.PureComponent {
filterOption: this.props.filterOption,
promptTextCreator: this.props.promptTextCreator,
ignoreAccents: false,
menuPortalTarget: this.props.menuPortalTarget,
menuPosition: this.props.menuPosition,
menuPlacement: this.props.menuPlacement,
};

let SelectComponent;
Expand Down

0 comments on commit 0893b97

Please sign in to comment.