superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From grace...@apache.org
Subject [incubator-superset] branch master updated: [Performance] VirtualizedSelect for SelectControl and FilterBox (#3654)
Date Mon, 06 Nov 2017 23:20:15 GMT
This is an automated email from the ASF dual-hosted git repository.

graceguo 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 9a49b1c  [Performance] VirtualizedSelect for SelectControl and FilterBox (#3654)
9a49b1c is described below

commit 9a49b1c41dca13b9f5095e7cc8f3b931d01accf9
Author: Jeff Niu <jeffniu22@gmail.com>
AuthorDate: Mon Nov 6 15:20:13 2017 -0800

    [Performance] VirtualizedSelect for SelectControl and FilterBox (#3654)
    
    * Added virtualized select to SelectControl, allow onPaste to create new options
    
    * Added unit tests
    
    * Added virtualized/paste select to filterbox
---
 .../javascripts/components/OnPasteSelect.jsx       |  87 +++++++++++++++++
 .../components/VirtualizedRendererWrap.jsx         |  56 +++++++++++
 .../explore/components/controls/SelectControl.jsx  |  65 ++-----------
 .../javascripts/components/OnPasteSelect_spec.jsx  | 105 ++++++++++++++++++++
 .../components/VirtualizedRendererWrap_spec.jsx    | 106 +++++++++++++++++++++
 .../explore/components/SelectControl_spec.jsx      |  33 ++++++-
 superset/assets/visualizations/filter_box.jsx      |  12 ++-
 7 files changed, 397 insertions(+), 67 deletions(-)

diff --git a/superset/assets/javascripts/components/OnPasteSelect.jsx b/superset/assets/javascripts/components/OnPasteSelect.jsx
new file mode 100644
index 0000000..b043bf3
--- /dev/null
+++ b/superset/assets/javascripts/components/OnPasteSelect.jsx
@@ -0,0 +1,87 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import Select from 'react-select';
+
+export default class OnPasteSelect extends React.Component {
+  onPaste(evt) {
+    if (!this.props.multi) {
+      return;
+    }
+    evt.preventDefault();
+    const clipboard = evt.clipboardData.getData('Text');
+    if (!clipboard) {
+      return;
+    }
+    const regex = `[${this.props.separator}]+`;
+    const values = clipboard.split(new RegExp(regex)).map(v => v.trim());
+    const validator = this.props.isValidNewOption;
+    const selected = this.props.value || [];
+    const existingOptions = {};
+    const existing = {};
+    this.props.options.forEach((v) => {
+      existingOptions[v[this.props.valueKey]] = 1;
+    });
+    let options = [];
+    selected.forEach((v) => {
+      options.push({ [this.props.labelKey]: v, [this.props.valueKey]: v });
+      existing[v] = 1;
+    });
+    options = options.concat(values
+      .filter((v) => {
+        const notExists = !existing[v];
+        existing[v] = 1;
+        return notExists && (validator ? validator({ [this.props.labelKey]: v })
: !!v);
+      })
+      .map((v) => {
+        const opt = { [this.props.labelKey]: v, [this.props.valueKey]: v };
+        if (!existingOptions[v]) {
+          this.props.options.unshift(opt);
+        }
+        return opt;
+      }),
+    );
+    if (options.length) {
+      if (this.props.onChange) {
+        this.props.onChange(options);
+      }
+    }
+  }
+  render() {
+    const SelectComponent = this.props.selectWrap;
+    const refFunc = (ref) => {
+      if (this.props.ref) {
+        this.props.ref(ref);
+      }
+      this.pasteInput = ref;
+    };
+    const inputProps = { onPaste: this.onPaste.bind(this) };
+    return (
+      <SelectComponent
+        {...this.props}
+        ref={refFunc}
+        inputProps={inputProps}
+      />
+    );
+  }
+}
+
+OnPasteSelect.propTypes = {
+  separator: PropTypes.string.isRequired,
+  selectWrap: PropTypes.func.isRequired,
+  ref: PropTypes.func,
+  onChange: PropTypes.func.isRequired,
+  valueKey: PropTypes.string.isRequired,
+  labelKey: PropTypes.string.isRequired,
+  options: PropTypes.array,
+  multi: PropTypes.bool.isRequired,
+  value: PropTypes.any,
+  isValidNewOption: PropTypes.func,
+};
+OnPasteSelect.defaultProps = {
+  separator: ',',
+  selectWrap: Select,
+  valueKey: 'value',
+  labelKey: 'label',
+  options: [],
+  multi: false,
+};
diff --git a/superset/assets/javascripts/components/VirtualizedRendererWrap.jsx b/superset/assets/javascripts/components/VirtualizedRendererWrap.jsx
new file mode 100644
index 0000000..3282112
--- /dev/null
+++ b/superset/assets/javascripts/components/VirtualizedRendererWrap.jsx
@@ -0,0 +1,56 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+
+export default function VirtualizedRendererWrap(renderer) {
+  function WrapperRenderer({
+    focusedOption,
+    focusOption,
+    key,
+    option,
+    selectValue,
+    style,
+    valueArray,
+  }) {
+    if (!option) {
+      return null;
+    }
+    const className = ['VirtualizedSelectOption'];
+    if (option === focusedOption) {
+      className.push('VirtualizedSelectFocusedOption');
+    }
+    if (option.disabled) {
+      className.push('VirtualizedSelectDisabledOption');
+    }
+    if (valueArray && valueArray.indexOf(option) >= 0) {
+      className.push('VirtualizedSelectSelectedOption');
+    }
+    if (option.className) {
+      className.push(option.className);
+    }
+    const events = option.disabled ? {} : {
+      onClick: () => selectValue(option),
+      onMouseEnter: () => focusOption(option),
+    };
+    return (
+      <div
+        className={className.join(' ')}
+        key={key}
+        style={Object.assign(option.style || {}, style)}
+        title={option.title}
+        {...events}
+      >
+        {renderer(option)}
+      </div>
+    );
+  }
+  WrapperRenderer.propTypes = {
+    focusedOption: PropTypes.object.isRequired,
+    focusOption: PropTypes.func.isRequired,
+    key: PropTypes.string,
+    option: PropTypes.object,
+    selectValue: PropTypes.func.isRequired,
+    style: PropTypes.object,
+    valueArray: PropTypes.array,
+  };
+  return WrapperRenderer;
+}
diff --git a/superset/assets/javascripts/explore/components/controls/SelectControl.jsx b/superset/assets/javascripts/explore/components/controls/SelectControl.jsx
index 51381d6..a82995d 100644
--- a/superset/assets/javascripts/explore/components/controls/SelectControl.jsx
+++ b/superset/assets/javascripts/explore/components/controls/SelectControl.jsx
@@ -1,8 +1,11 @@
 import React from 'react';
 import PropTypes from 'prop-types';
+import VirtualizedSelect from 'react-virtualized-select';
 import Select, { Creatable } from 'react-select';
 import ControlHeader from '../ControlHeader';
 import { t } from '../../../locales';
+import VirtualizedRendererWrap from '../../../components/VirtualizedRendererWrap';
+import OnPasteSelect from '../../../components/OnPasteSelect';
 
 const propTypes = {
   choices: PropTypes.array,
@@ -37,55 +40,6 @@ const defaultProps = {
   valueKey: 'value',
 };
 
-// Handle `onPaste` so that users may paste in
-// options as comma-delimited, slightly modified from
-// https://github.com/JedWatson/react-select/issues/1672
-function pasteSelect(props) {
-  let pasteInput;
-  return (
-    <Select
-      {...props}
-      ref={(ref) => {
-        // Creatable requires a reference to its Select child
-        if (props.ref) {
-          props.ref(ref);
-        }
-        pasteInput = ref;
-      }}
-      inputProps={{
-        onPaste: (evt) => {
-          if (!props.multi) {
-            return;
-          }
-          evt.preventDefault();
-          // pull text from the clipboard and split by comma
-          const clipboard = evt.clipboardData.getData('Text');
-          if (!clipboard) {
-            return;
-          }
-          const values = clipboard.split(/[,]+/).map(v => v.trim());
-          const options = values
-            .filter(value =>
-              // Creatable validates options
-              props.isValidNewOption ? props.isValidNewOption({ label: value }) : !!value,
-            )
-            .map(value => ({
-              [props.labelKey]: value,
-              [props.valueKey]: value,
-            }));
-          if (options.length) {
-            pasteInput.selectValue(options);
-          }
-        },
-      }}
-    />
-  );
-}
-pasteSelect.propTypes = {
-  multi: PropTypes.bool,
-  ref: PropTypes.func,
-};
-
 export default class SelectControl extends React.PureComponent {
   constructor(props) {
     super(props);
@@ -161,23 +115,16 @@ export default class SelectControl extends React.PureComponent {
       clearable: this.props.clearable,
       isLoading: this.props.isLoading,
       onChange: this.onChange,
-      optionRenderer: this.props.optionRenderer,
+      optionRenderer: VirtualizedRendererWrap(this.props.optionRenderer),
       valueRenderer: this.props.valueRenderer,
+      selectComponent: this.props.freeForm ? Creatable : Select,
     };
-    //  Tab, comma or Enter will trigger a new option created for FreeFormSelect
-    const selectWrap = this.props.freeForm ? (
-      <Creatable {...selectProps}>
-        {pasteSelect}
-      </Creatable>
-    ) : (
-      pasteSelect(selectProps)
-    );
     return (
       <div>
         {this.props.showHeader &&
           <ControlHeader {...this.props} />
         }
-        {selectWrap}
+        <OnPasteSelect {...selectProps} selectWrap={VirtualizedSelect} />
       </div>
     );
   }
diff --git a/superset/assets/spec/javascripts/components/OnPasteSelect_spec.jsx b/superset/assets/spec/javascripts/components/OnPasteSelect_spec.jsx
new file mode 100644
index 0000000..949ca03
--- /dev/null
+++ b/superset/assets/spec/javascripts/components/OnPasteSelect_spec.jsx
@@ -0,0 +1,105 @@
+/* eslint-disable no-unused-expressions */
+import React from 'react';
+import sinon from 'sinon';
+import { expect } from 'chai';
+import { shallow } from 'enzyme';
+import { describe, it } from 'mocha';
+import VirtualizedSelect from 'react-virtualized-select';
+import Select, { Creatable } from 'react-select';
+
+import OnPasteSelect from '../../../javascripts/components/OnPasteSelect';
+
+const defaultProps = {
+  onChange: sinon.spy(),
+  multi: true,
+  isValidNewOption: sinon.spy(s => !!s.label),
+  value: [],
+  options: [
+    { value: 'United States', label: 'United States' },
+    { value: 'China', label: 'China' },
+    { value: 'India', label: 'India' },
+    { value: 'Canada', label: 'Canada' },
+    { value: 'Russian Federation', label: 'Russian Federation' },
+    { value: 'Japan', label: 'Japan' },
+    { value: 'Mexico', label: 'Mexico' },
+  ],
+};
+
+const defaultEvt = {
+  preventDefault: sinon.spy(),
+  clipboardData: {
+    getData: sinon.spy(() => ' United States, China  , India, Canada, '),
+  },
+};
+
+describe('OnPasteSelect', () => {
+  let wrapper;
+  let props;
+  let evt;
+  let expected;
+  beforeEach(() => {
+    props = Object.assign({}, defaultProps);
+    wrapper = shallow(<OnPasteSelect {...props} />);
+    evt = Object.assign({}, defaultEvt);
+  });
+
+  it('renders the supplied selectWrap component', () => {
+    const select = wrapper.find(Select);
+    expect(select).to.have.lengthOf(1);
+  });
+
+  it('renders custom selectWrap components', () => {
+    props.selectWrap = Creatable;
+    wrapper = shallow(<OnPasteSelect {...props} />);
+    expect(wrapper.find(Creatable)).to.have.lengthOf(1);
+    props.selectWrap = VirtualizedSelect;
+    wrapper = shallow(<OnPasteSelect {...props} />);
+    expect(wrapper.find(VirtualizedSelect)).to.have.lengthOf(1);
+  });
+
+  describe('onPaste', () => {
+    it('calls onChange with pasted values', () => {
+      wrapper.instance().onPaste(evt);
+      expected = props.options.slice(0, 4);
+      expect(props.onChange.calledWith(expected)).to.be.true;
+      expect(evt.preventDefault.called).to.be.true;
+      expect(props.isValidNewOption.callCount).to.equal(5);
+    });
+
+    it('calls onChange without any duplicate values and adds new values', () => {
+      evt.clipboardData.getData = sinon.spy(() =>
+        'China, China, China, China, Mexico, Mexico, Chi na, Mexico, ',
+      );
+      expected = [
+        props.options[1],
+        props.options[6],
+        { label: 'Chi na', value: 'Chi na' },
+      ];
+      wrapper.instance().onPaste(evt);
+      expect(props.onChange.calledWith(expected)).to.be.true;
+      expect(evt.preventDefault.called).to.be.true;
+      expect(props.isValidNewOption.callCount).to.equal(9);
+      expect(props.options[0].value).to.equal(expected[2].value);
+      props.options.splice(0, 1);
+    });
+
+    it('calls onChange with currently selected values and new values', () => {
+      props.value = ['United States', 'Canada', 'Mexico'];
+      evt.clipboardData.getData = sinon.spy(() =>
+        'United States, Canada, Japan, India',
+      );
+      wrapper = shallow(<OnPasteSelect {...props} />);
+      expected = [
+        props.options[0],
+        props.options[3],
+        props.options[6],
+        props.options[5],
+        props.options[2],
+      ];
+      wrapper.instance().onPaste(evt);
+      expect(props.onChange.calledWith(expected)).to.be.true;
+      expect(evt.preventDefault.called).to.be.true;
+      expect(props.isValidNewOption.callCount).to.equal(11);
+    });
+  });
+});
diff --git a/superset/assets/spec/javascripts/components/VirtualizedRendererWrap_spec.jsx
b/superset/assets/spec/javascripts/components/VirtualizedRendererWrap_spec.jsx
new file mode 100644
index 0000000..1cd4c4e
--- /dev/null
+++ b/superset/assets/spec/javascripts/components/VirtualizedRendererWrap_spec.jsx
@@ -0,0 +1,106 @@
+/* eslint-disable no-unused-expressions */
+import React from 'react';
+import sinon from 'sinon';
+import PropTypes from 'prop-types';
+import { expect } from 'chai';
+import { describe, it } from 'mocha';
+import { shallow } from 'enzyme';
+
+import VirtualizedRendererWrap from '../../../javascripts/components/VirtualizedRendererWrap';
+
+const defaultProps = {
+  focusedOption: { label: 'focusedOn', value: 'focusedOn' },
+  focusOption: sinon.spy(),
+  key: 'key1',
+  option: { label: 'option1', value: 'option1' },
+  selectValue: sinon.spy(),
+  valueArray: [],
+};
+
+function TestOption({ option }) {
+  return (
+    <span>{option.label}</span>
+  );
+}
+TestOption.propTypes = {
+  option: PropTypes.object.isRequired,
+};
+
+const defaultRenderer = opt => <TestOption option={opt} />;
+const RendererWrap = VirtualizedRendererWrap(defaultRenderer);
+
+describe('VirtualizedRendererWrap', () => {
+  let wrapper;
+  let props;
+  beforeEach(() => {
+    wrapper = shallow(<RendererWrap {...defaultProps} />);
+    props = Object.assign({}, defaultProps);
+  });
+
+  it('uses the provided renderer', () => {
+    const option = wrapper.find(TestOption);
+    expect(option).to.have.lengthOf(1);
+  });
+
+  it('renders nothing when no option is provided', () => {
+    props.option = null;
+    wrapper = shallow(<RendererWrap {...props} />);
+    const option = wrapper.find(TestOption);
+    expect(option).to.have.lengthOf(0);
+  });
+
+  it('renders unfocused, unselected options with the default class', () => {
+    const optionDiv = wrapper.find('div');
+    expect(optionDiv).to.have.lengthOf(1);
+    expect(optionDiv.props().className).to.equal('VirtualizedSelectOption');
+  });
+
+  it('renders focused option with the correct class', () => {
+    props.option = props.focusedOption;
+    wrapper = shallow(<RendererWrap {...props} />);
+    const optionDiv = wrapper.find('div');
+    expect(optionDiv.props().className).to.equal(
+      'VirtualizedSelectOption VirtualizedSelectFocusedOption',
+    );
+  });
+
+  it('renders disabled option with the correct class', () => {
+    props.option.disabled = true;
+    wrapper = shallow(<RendererWrap {...props} />);
+    const optionDiv = wrapper.find('div');
+    expect(optionDiv.props().className).to.equal(
+      'VirtualizedSelectOption VirtualizedSelectDisabledOption',
+    );
+    props.option.disabled = false;
+  });
+
+  it('renders selected option with the correct class', () => {
+    props.valueArray = [props.option, props.focusedOption];
+    wrapper = shallow(<RendererWrap {...props} />);
+    const optionDiv = wrapper.find('div');
+    expect(optionDiv.props().className).to.equal(
+      'VirtualizedSelectOption VirtualizedSelectSelectedOption',
+    );
+  });
+
+  it('renders options with custom classes', () => {
+    props.option.className = 'CustomClass';
+    wrapper = shallow(<RendererWrap {...props} />);
+    const optionDiv = wrapper.find('div');
+    expect(optionDiv.props().className).to.equal(
+      'VirtualizedSelectOption CustomClass',
+    );
+  });
+
+  it('calls focusedOption on its own option onMouseEnter', () => {
+    const optionDiv = wrapper.find('div');
+    optionDiv.simulate('mouseEnter');
+    expect(props.focusOption.calledWith(props.option)).to.be.true;
+  });
+
+  it('calls selectValue on its own option onClick', () => {
+    const optionDiv = wrapper.find('div');
+    optionDiv.simulate('click');
+    expect(props.selectValue.calledWith(props.option)).to.be.true;
+  });
+});
diff --git a/superset/assets/spec/javascripts/explore/components/SelectControl_spec.jsx b/superset/assets/spec/javascripts/explore/components/SelectControl_spec.jsx
index 7508eeb..d0eea9c 100644
--- a/superset/assets/spec/javascripts/explore/components/SelectControl_spec.jsx
+++ b/superset/assets/spec/javascripts/explore/components/SelectControl_spec.jsx
@@ -1,10 +1,13 @@
 /* eslint-disable no-unused-expressions */
 import React from 'react';
 import Select, { Creatable } from 'react-select';
+import VirtualizedSelect from 'react-virtualized-select';
 import sinon from 'sinon';
 import { expect } from 'chai';
 import { describe, it, beforeEach } from 'mocha';
 import { shallow } from 'enzyme';
+import OnPasteSelect from '../../../../javascripts/components/OnPasteSelect';
+import VirtualizedRendererWrap from '../../../../javascripts/components/VirtualizedRendererWrap';
 import SelectControl from '../../../../javascripts/explore/components/controls/SelectControl';
 
 const defaultProps = {
@@ -26,19 +29,39 @@ describe('SelectControl', () => {
     wrapper = shallow(<SelectControl {...defaultProps} />);
   });
 
-  it('renders a Select', () => {
-    expect(wrapper.find(Select)).to.have.lengthOf(1);
+  it('renders an OnPasteSelect', () => {
+    expect(wrapper.find(OnPasteSelect)).to.have.lengthOf(1);
   });
 
   it('calls onChange when toggled', () => {
-    const select = wrapper.find(Select);
+    const select = wrapper.find(OnPasteSelect);
     select.simulate('change', { value: 50 });
     expect(defaultProps.onChange.calledWith(50)).to.be.true;
   });
 
-  it('renders a Creatable for freeform', () => {
+  it('passes VirtualizedSelect as selectWrap', () => {
+    const select = wrapper.find(OnPasteSelect);
+    expect(select.props().selectWrap).to.equal(VirtualizedSelect);
+  });
+
+  it('passes Creatable as selectComponent when freeForm=true', () => {
     wrapper = shallow(<SelectControl {...defaultProps} freeForm />);
-    expect(wrapper.find(Creatable)).to.have.lengthOf(1);
+    const select = wrapper.find(OnPasteSelect);
+    expect(select.props().selectComponent).to.equal(Creatable);
+  });
+
+  it('passes Select as selectComponent when freeForm=false', () => {
+    const select = wrapper.find(OnPasteSelect);
+    expect(select.props().selectComponent).to.equal(Select);
+  });
+
+  it('wraps optionRenderer in a VirtualizedRendererWrap', () => {
+    const select = wrapper.find(OnPasteSelect);
+    const defaultOptionRenderer = SelectControl.defaultProps.optionRenderer;
+    const wrappedRenderer = VirtualizedRendererWrap(defaultOptionRenderer);
+    expect(select.props().optionRenderer).to.be.a('Function');
+    // different instances of wrapper with same inner renderer are unequal
+    expect(select.props().optionRenderer.name).to.equal(wrappedRenderer.name);
   });
 
   describe('getOptions', () => {
diff --git a/superset/assets/visualizations/filter_box.jsx b/superset/assets/visualizations/filter_box.jsx
index d09755a..bdcf978 100644
--- a/superset/assets/visualizations/filter_box.jsx
+++ b/superset/assets/visualizations/filter_box.jsx
@@ -3,13 +3,16 @@ import d3 from 'd3';
 import React from 'react';
 import PropTypes from 'prop-types';
 import ReactDOM from 'react-dom';
-import Select from 'react-select';
+import VirtualizedSelect from 'react-virtualized-select';
+import { Creatable } from 'react-select';
 import { Button } from 'react-bootstrap';
 
 import DateFilterControl from '../javascripts/explore/components/controls/DateFilterControl';
 import ControlRow from '../javascripts/explore/components/ControlRow';
 import Control from '../javascripts/explore/components/Control';
 import controls from '../javascripts/explore/stores/controls';
+import OnPasteSelect from '../javascripts/components/OnPasteSelect';
+import VirtualizedRendererWrap from '../javascripts/components/VirtualizedRendererWrap';
 import './filter_box.css';
 import { t } from '../javascripts/locales';
 
@@ -164,7 +167,7 @@ class FilterBox extends React.Component {
             text: v,
             metric: 0,
           };
-          this.props.filtersChoices[filterKey].push(addChoice);
+          this.props.filtersChoices[filterKey].unshift(addChoice);
         }
       }
     }
@@ -177,7 +180,7 @@ class FilterBox extends React.Component {
       return (
         <div key={filter} className="m-b-5">
           {this.props.datasource.verbose_map[filter] || filter}
-          <Select.Creatable
+          <OnPasteSelect
             placeholder={t('Select [%s]', filter)}
             key={filter}
             multi
@@ -195,6 +198,9 @@ class FilterBox extends React.Component {
               return { value: opt.id, label: opt.id, style };
             })}
             onChange={this.changeFilter.bind(this, filter)}
+            selectComponent={Creatable}
+            selectWrap={VirtualizedSelect}
+            optionRenderer={VirtualizedRendererWrap(opt => opt.label)}
           />
         </div>
       );

-- 
To stop receiving notification emails like this one, please contact
['"commits@superset.apache.org" <commits@superset.apache.org>'].

Mime
View raw message