From 0893b97f0a1c95d0317636ba8cf009338f5fc4ab Mon Sep 17 00:00:00 2001
From: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Date: Fri, 6 Nov 2020 05:00:43 +0100
Subject: [PATCH] fix: Issues with filters and metrics popovers (#11578)

* Fix bugs in AdhocFilterEditPopover

* Fix bugs in AdhocMetricEditPopover

* Remove handleMultiComparatorInputHeightChange function

* Fix tests
---
 ...FilterEditPopoverSimpleTabContent_spec.jsx | 12 -----
 .../AdhocFilterEditPopover_spec.jsx           |  4 +-
 .../AdhocMetricEditPopover_spec.jsx           |  4 +-
 .../components/AdhocFilterEditPopover.jsx     | 14 +++--
 ...AdhocFilterEditPopoverSimpleTabContent.jsx | 51 +++++--------------
 .../AdhocFilterEditPopoverSqlTabContent.jsx   |  2 +-
 .../components/AdhocMetricEditPopover.jsx     | 43 +++++++++++-----
 .../components/controls/SelectControl.jsx     |  6 +++
 8 files changed, 64 insertions(+), 72 deletions(-)

diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx
index f58588e0b614c..442527b77b4aa 100644
--- a/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx
@@ -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);
-  });
 });
diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopover_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopover_spec.jsx
index c28ac9a425670..606e0930cb5ad 100644
--- a/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopover_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopover_spec.jsx
@@ -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);
   });
 });
diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
index 4886760a6222e..5aff466065f2e 100644
--- a/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
@@ -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);
   });
 });
diff --git a/superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx b/superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx
index 11b5b6924a4fa..e12adb9fa8ab9 100644
--- a/superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx
+++ b/superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx
@@ -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';
@@ -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 {
@@ -63,6 +67,8 @@ export default class AdhocFilterEditPopover extends React.Component {
       width: startingWidth,
       height: startingHeight,
     };
+
+    this.popoverContentRef = React.createRef();
   }
 
   componentDidMount() {
@@ -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"
@@ -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
@@ -195,7 +203,7 @@ export default class AdhocFilterEditPopover extends React.Component {
           >
             {t('Save')}
           </Button>
-          <i
+          <ResizeIcon
             role="button"
             aria-label="Resize"
             tabIndex={0}
diff --git a/superset-frontend/src/explore/components/AdhocFilterEditPopoverSimpleTabContent.jsx b/superset-frontend/src/explore/components/AdhocFilterEditPopoverSimpleTabContent.jsx
index 8d1ee4961abdd..07d29334bc5c4 100644
--- a/superset-frontend/src/explore/components/AdhocFilterEditPopoverSimpleTabContent.jsx
+++ b/superset-frontend/src/explore/components/AdhocFilterEditPopoverSimpleTabContent.jsx
@@ -51,6 +51,7 @@ const propTypes = {
   onHeightChange: PropTypes.func.isRequired,
   datasource: PropTypes.object,
   partitionColumn: PropTypes.string,
+  popoverRef: PropTypes.object,
 };
 
 const defaultProps = {
@@ -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);
@@ -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,
     };
 
@@ -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) {
@@ -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;
@@ -270,12 +247,6 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
     }
   }
 
-  multiComparatorRef(ref) {
-    if (ref) {
-      this.multiComparatorComponent = ref;
-    }
-  }
-
   renderSubjectOptionLabel(option) {
     return <FilterDefinitionOption option={option} />;
   }
@@ -328,10 +299,11 @@ 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"
           />
@@ -339,6 +311,7 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
         <FormGroup>
           <Select
             {...this.selectProps}
+            {...this.menuPortalProps}
             {...operatorSelectProps}
             name="filter-operator"
           />
@@ -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
@@ -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)}
             />
           ) : (
@@ -373,7 +346,7 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
             />
           )}
         </FormGroup>
-      </span>
+      </>
     );
   }
 }
diff --git a/superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx b/superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx
index 702c46d8476ae..2a982bacec978 100644
--- a/superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx
+++ b/superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx
@@ -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}
diff --git a/superset-frontend/src/explore/components/AdhocMetricEditPopover.jsx b/superset-frontend/src/explore/components/AdhocMetricEditPopover.jsx
index d53d9dff847ab..2bc1f1e383c44 100644
--- a/superset-frontend/src/explore/components/AdhocMetricEditPopover.jsx
+++ b/superset-frontend/src/explore/components/AdhocMetricEditPopover.jsx
@@ -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';
@@ -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 {
@@ -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);
   }
 
@@ -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
@@ -237,6 +253,7 @@ export default class AdhocMetricEditPopover extends React.Component {
               <Select
                 name="select-column"
                 {...this.selectProps}
+                {...this.menuPortalProps}
                 {...columnSelectProps}
               />
             </FormGroup>
@@ -247,6 +264,7 @@ export default class AdhocMetricEditPopover extends React.Component {
               <Select
                 name="select-aggregate"
                 {...this.selectProps}
+                {...this.menuPortalProps}
                 {...aggregateSelectProps}
                 autoFocus
               />
@@ -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}
@@ -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}
diff --git a/superset-frontend/src/explore/components/controls/SelectControl.jsx b/superset-frontend/src/explore/components/controls/SelectControl.jsx
index 7ec6c699215c5..defe877434775 100644
--- a/superset-frontend/src/explore/components/controls/SelectControl.jsx
+++ b/superset-frontend/src/explore/components/controls/SelectControl.jsx
@@ -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 = {
@@ -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;