Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Select component refactoring - SelectControl - Iteration 5 #16510

Merged
merged 35 commits into from
Oct 4, 2021
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
5e02cfd
Refactor Select DatasourceEditor
geido Aug 29, 2021
8297208
Fire onChange with allowNewOptions
geido Aug 29, 2021
0885f6c
Clean up
geido Aug 29, 2021
8c3b96b
Refactor Select in AnnotationLayer
geido Aug 29, 2021
16341a7
Handle on clear
geido Aug 30, 2021
5ed953c
Update tests
geido Aug 30, 2021
67a2332
Refactor Select in SpatialControl
geido Aug 30, 2021
baa04dc
Show search
geido Aug 30, 2021
46be115
Refactor Select in FilterBox
geido Aug 30, 2021
75e2dcb
Remove search where unnecessary
geido Aug 30, 2021
c1a867f
Update SelectControl - WIP
geido Aug 30, 2021
9b0ec18
Refactor Controls
geido Aug 31, 2021
d89fabe
Update SelectControl tests
geido Aug 31, 2021
39d86c9
Clean up
geido Aug 31, 2021
ffcb26b
Test allowNewOptions false
geido Aug 31, 2021
a965366
Use SelectControl AnnotationLayer
geido Aug 31, 2021
3118a55
Use SelectControl SpatialControl
geido Aug 31, 2021
916fc29
Clean up
geido Aug 31, 2021
a17164b
Merge branch 'master' of https://github.com/apache/superset into chor…
geido Aug 31, 2021
e48e428
Render custom label
geido Aug 31, 2021
738ee0d
Show search
geido Aug 31, 2021
fb13337
Implement filterOption
geido Aug 31, 2021
72810cd
Improve filterOption
geido Aug 31, 2021
bb7b470
Update Cypress
geido Aug 31, 2021
7706a71
Update Cypress table test
geido Aug 31, 2021
5854d0f
Use value for defaultValue
geido Sep 7, 2021
30e6b41
Merge
geido Sep 7, 2021
d69e33f
Merge with latest changes
geido Sep 7, 2021
eb0f368
Merge branch 'master' of https://github.com/apache/superset into chor…
geido Sep 16, 2021
7bfcd93
Reconcile with latest Select changes
geido Sep 16, 2021
5a94d56
Update superset-frontend/src/explore/components/controls/AnnotationLa…
geido Sep 21, 2021
3a5ee3f
Update superset-frontend/src/explore/components/controls/AnnotationLa…
geido Sep 21, 2021
353ae00
Revert changes to test
geido Sep 21, 2021
21dc30e
Call onPopoverClear when v value is undefined
geido Sep 23, 2021
748af87
Remove unnecessary onClear checks
geido Sep 23, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ describe('Advanced analytics', () => {

cy.get('.ant-collapse-header').contains('Advanced Analytics').click();

cy.get('[data-test=time_compare]').find('.Select__control').click();
cy.get('[data-test=time_compare]').find('.ant-select').click();
cy.get('[data-test=time_compare]')
.find('input[type=text]')
.find('input[type=search]')
.type('28 days{enter}');

cy.get('[data-test=time_compare]')
.find('input[type=text]')
.find('input[type=search]')
.type('1 year{enter}');

cy.get('button[data-test="run-query-button"]').click();
Expand All @@ -48,10 +48,10 @@ describe('Advanced analytics', () => {

cy.get('.ant-collapse-header').contains('Advanced Analytics').click();
cy.get('[data-test=time_compare]')
.find('.Select__multi-value__label')
.find('.ant-select-selector')
.contains('28 days');
cy.get('[data-test=time_compare]')
.find('.Select__multi-value__label')
.find('.ant-select-selector')
.contains('1 year');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,8 @@ describe('Groupby control', () => {
cy.verifySliceSuccess({ waitAlias: '@chartData' });

cy.get('[data-test=groupby]').within(() => {
cy.get('.Select__control').click();
cy.get('input[type=text]').type('state{enter}');
cy.get('.ant-select').click();
cy.get('input[type=search]').type('state{enter}');
});
cy.get('button[data-test="run-query-button"]').click();
cy.verifySliceSuccess({ waitAlias: '@chartData', chartSelector: 'svg' });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe('Visualization > Table', () => {
granularity_sqla: undefined,
metrics: ['count'],
});
cy.get('input[name="select-granularity_sqla"]').should('have.value', 'ds');
cy.get('[data-test=granularity_sqla] .column-option-label').contains('ds');
});

it('Format non-numeric metrics correctly', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@
import React from 'react';
import sinon from 'sinon';
import { shallow } from 'enzyme';
import { Select, CreatableSelect } from 'src/components/Select';
import OnPasteSelect from 'src/components/Select/OnPasteSelect';
import { Select as SelectComponent } from 'src/components';
import SelectControl from 'src/explore/components/controls/SelectControl';
import { styledMount as mount } from 'spec/helpers/theming';

Expand All @@ -48,59 +47,35 @@ describe('SelectControl', () => {
wrapper = shallow(<SelectControl {...defaultProps} />);
});

it('uses Select in onPasteSelect when freeForm=false', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OnPasteSelect is not used with the new Select component. These tests are included later checking whether allowNewOptions is true or false based on the SelectControl freeForm option.

wrapper = shallow(<SelectControl {...defaultProps} multi />);
const select = wrapper.find(OnPasteSelect);
expect(select.props().selectWrap).toBe(Select);
});

it('uses Creatable in onPasteSelect when freeForm=true', () => {
wrapper = shallow(<SelectControl {...defaultProps} multi freeForm />);
const select = wrapper.find(OnPasteSelect);
expect(select.props().selectWrap).toBe(CreatableSelect);
});

it('calls props.onChange when select', () => {
const select = wrapper.instance();
select.onChange({ value: 50 });
select.onChange(50);
expect(defaultProps.onChange.calledWith(50)).toBe(true);
});

it('returns all options on select all', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Select all" option has been removed as agreed with design and all the related tests have been removed as well

const expectedValues = ['one', 'two'];
const selectAllProps = {
multi: true,
allowAll: true,
choices: expectedValues,
name: 'row_limit',
label: 'Row Limit',
valueKey: 'value',
onChange: sinon.spy(),
};
wrapper.setProps(selectAllProps);
wrapper.instance().onChange([{ meta: true, value: 'Select all' }]);
expect(selectAllProps.onChange.calledWith(expectedValues)).toBe(true);
});

describe('render', () => {
it('renders with Select by default', () => {
expect(wrapper.find(OnPasteSelect)).not.toExist();
expect(wrapper.findWhere(x => x.type() === Select)).toHaveLength(1);
expect(wrapper.find(SelectComponent)).toExist();
});

it('renders with OnPasteSelect when multi', () => {
it('renders as mode multiple', () => {
wrapper.setProps({ multi: true });
expect(wrapper.find(OnPasteSelect)).toExist();
expect(wrapper.findWhere(x => x.type() === Select)).toHaveLength(0);
expect(wrapper.find(SelectComponent)).toExist();
expect(wrapper.find(SelectComponent).prop('mode')).toBe('multiple');
});

it('renders with Creatable when freeForm', () => {
it('renders with allowNewOptions when freeForm', () => {
wrapper.setProps({ freeForm: true });
expect(wrapper.find(OnPasteSelect)).not.toExist();
expect(wrapper.findWhere(x => x.type() === CreatableSelect)).toHaveLength(
1,
);
expect(wrapper.find(SelectComponent)).toExist();
expect(wrapper.find(SelectComponent).prop('allowNewOptions')).toBe(true);
});

it('renders with allowNewOptions=false when freeForm=false', () => {
wrapper.setProps({ freeForm: false });
expect(wrapper.find(SelectComponent)).toExist();
expect(wrapper.find(SelectComponent).prop('allowNewOptions')).toBe(false);
});

describe('empty placeholder', () => {
describe('withMulti', () => {
it('does not show a placeholder if there are no choices', () => {
Expand Down Expand Up @@ -161,16 +136,6 @@ describe('SelectControl', () => {
);
expect(wrapper.html()).not.toContain('add something');
});
it('shows numbers of options as a placeholder by default', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number of options and remaining options has been removed as agreed with design and all the related tests have been removed as well

wrapper = mount(<SelectControl {...defaultProps} multi />);
expect(wrapper.html()).toContain('2 option(s');
});
it('reduces the number of options in the placeholder by the value length', () => {
wrapper = mount(
<SelectControl {...defaultProps} multi value={['today']} />,
);
expect(wrapper.html()).toContain('1 option(s');
});
});
describe('when select is single', () => {
it('does not render the placeholder when a selection has been made', () => {
Expand All @@ -186,82 +151,12 @@ describe('SelectControl', () => {
});
});

describe('optionsRemaining', () => {
describe('isMulti', () => {
it('returns the options minus selected values', () => {
const wrapper = mount(
<SelectControl {...defaultProps} multi value={['today']} />,
);
expect(wrapper.instance().optionsRemaining()).toEqual(1);
});
});
describe('is not multi', () => {
it('returns the length of all options', () => {
wrapper = mount(
<SelectControl
{...defaultProps}
value={50}
placeholder="add something"
/>,
);
expect(wrapper.instance().optionsRemaining()).toEqual(2);
});
});
describe('with Select all', () => {
it('does not count it', () => {
const props = { ...defaultProps, multi: true, allowAll: true };
const wrapper = mount(<SelectControl {...props} />);
expect(wrapper.instance().getOptions(props).length).toEqual(3);
expect(wrapper.instance().optionsRemaining()).toEqual(2);
});
});
});

describe('getOptions', () => {
it('returns the correct options', () => {
wrapper.setProps(defaultProps);
expect(wrapper.instance().getOptions(defaultProps)).toEqual(options);
});

it('shows Select-All when enabled', () => {
const selectAllProps = {
choices: ['one', 'two'],
name: 'name',
freeForm: true,
allowAll: true,
multi: true,
valueKey: 'value',
};
wrapper.setProps(selectAllProps);
expect(wrapper.instance().getOptions(selectAllProps)).toContainEqual({
label: 'Select all',
meta: true,
value: 'Select all',
});
});

it('returns the correct options when freeform is set to true', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not necessary with the new Select component as it is a standard behavior

const freeFormProps = {
choices: [],
freeForm: true,
value: ['one', 'two'],
name: 'row_limit',
label: 'Row Limit',
valueKey: 'custom_value_key',
onChange: sinon.spy(),
};
// the last added option is at the top
const expectedNewOptions = [
{ custom_value_key: 'two', label: 'two' },
{ custom_value_key: 'one', label: 'one' },
];
wrapper.setProps(freeFormProps);
expect(wrapper.instance().getOptions(freeFormProps)).toEqual(
expectedNewOptions,
);
});
});

describe('UNSAFE_componentWillReceiveProps', () => {
it('sets state.options if props.choices has changed', () => {
const updatedOptions = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const defaultProps = {
onChange: sinon.spy(),
};

describe('SelectControl', () => {
describe('TextArea', () => {
let wrapper;
beforeEach(() => {
wrapper = shallow(<TextAreaControl {...defaultProps} />);
Expand Down
3 changes: 0 additions & 3 deletions superset-frontend/spec/javascripts/explore/fixtures.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,10 @@ export const controlPanelSectionsChartOptionsTable: ControlPanelSectionConfig[]
default: [],
description: t('Columns to display'),
optionRenderer: c => <ColumnOption column={c} showType />,
valueRenderer: c => <ColumnOption column={c} />,
valueKey: 'column_name',
allowAll: true,
mapStateToProps: stateRef => ({
options: stateRef.datasource ? stateRef.datasource.columns : [],
}),
commaChoosesOption: false,
freeForm: true,
} as ControlConfig<'SelectControl', ColumnMeta>,
},
Expand Down
17 changes: 14 additions & 3 deletions superset-frontend/src/datasource/DatasourceEditor.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import { getClientErrorObject } from 'src/utils/getClientErrorObject';

import CheckboxControl from 'src/explore/components/controls/CheckboxControl';
import TextControl from 'src/explore/components/controls/TextControl';
import SelectControl from 'src/explore/components/controls/SelectControl';
import { Select } from 'src/components';
import TextAreaControl from 'src/explore/components/controls/TextAreaControl';
import SelectAsyncControl from 'src/explore/components/controls/SelectAsyncControl';
import SpatialControl from 'src/explore/components/controls/SpatialControl';
Expand Down Expand Up @@ -113,7 +113,12 @@ const ColumnButtonWrapper = styled.div`
const checkboxGenerator = (d, onChange) => (
<CheckboxControl value={d} onChange={onChange} />
);
const DATA_TYPES = ['STRING', 'NUMERIC', 'DATETIME', 'BOOLEAN'];
const DATA_TYPES = [
{ value: 'STRING', label: 'STRING' },
{ value: 'NUMERIC', label: 'NUMERIC' },
{ value: 'DATETIME', label: 'DATETIME' },
{ value: 'BOOLEAN', label: 'BOOLEAN' },
];
Comment on lines +124 to +129
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but this probably shouldn't be a Select component at all, but rather a text field. These should be raw column types coming from the database, hence a typical STRING will be VARCHAR(x), TEXT etc.


const DATASOURCE_TYPES_ARR = [
{ key: 'physical', label: t('Physical (table or view)') },
Expand Down Expand Up @@ -199,7 +204,13 @@ function ColumnCollectionTable({
fieldKey="type"
label={t('Data type')}
control={
<SelectControl choices={DATA_TYPES} name="type" freeForm />
<Select
ariaLabel={t('Data type')}
options={DATA_TYPES}
name="type"
allowNewOptions
allowClear
/>
}
/>
)}
Expand Down
Loading