From f476ba23a279cb87a94ad3075e035cad0ae264b6 Mon Sep 17 00:00:00 2001 From: Corbin Robb <31329271+corbinrobb@users.noreply.github.com> Date: Thu, 9 Dec 2021 09:40:25 -0700 Subject: [PATCH] fix(select): select component sort functionality on certain options (#17638) * fix: Select component sort function sorting by label instead of value on numbers * fix: change select component default sorting to sort by the initial index rather than a property like value or label * fix: select sorting add sortOptions to select components using sortByProperty * fix: change select component back, add order to options coming in from SelectControl * fix: select component options intitial sort bug * fix: add test cases for select fix Co-authored-by: Corbin Robb --- .../src/shared-controls/index.tsx | 2 + .../src/controlPanel.ts | 4 +- .../plugin-chart-table/src/controlPanel.tsx | 4 ++ .../explore/components/SelectControl_spec.jsx | 41 +++++++++++++++++-- .../src/components/Select/Select.tsx | 7 ++-- .../index.tsx | 8 +++- .../components/controls/SelectControl.jsx | 6 ++- 7 files changed, 59 insertions(+), 13 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx index 6017d50b99..1c9ea8d8d4 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx @@ -106,6 +106,8 @@ const groupByControl: SharedControlConfig<'SelectControl', ColumnMeta> = { 'One or many columns to group by. High cardinality groupings should include a sort by metric ' + 'and series limit to limit the number of fetched and rendered series.', ), + sortComparator: (a: { label: string }, b: { label: string }) => + a.label.localeCompare(b.label), optionRenderer: c => , valueRenderer: c => , valueKey: 'column_name', diff --git a/superset-frontend/plugins/legacy-plugin-chart-heatmap/src/controlPanel.ts b/superset-frontend/plugins/legacy-plugin-chart-heatmap/src/controlPanel.ts index 6fb5bfd085..b270d43e35 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-heatmap/src/controlPanel.ts +++ b/superset-frontend/plugins/legacy-plugin-chart-heatmap/src/controlPanel.ts @@ -114,7 +114,7 @@ const config: ControlPanelConfig = { label: t('XScale Interval'), renderTrigger: true, choices: formatSelectOptionsForRange(1, 50), - default: '1', + default: 1, clearable: false, description: t( 'Number of steps to take between ticks when displaying the X scale', @@ -129,7 +129,7 @@ const config: ControlPanelConfig = { type: 'SelectControl', label: t('YScale Interval'), choices: formatSelectOptionsForRange(1, 50), - default: '1', + default: 1, clearable: false, renderTrigger: true, description: t( diff --git a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx index 6509368961..2afb1698c7 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx @@ -116,6 +116,8 @@ const all_columns: typeof sharedControls.groupby = { ? [t('must have a value')] : [], }), + sortComparator: (a: { label: string }, b: { label: string }) => + a.label.localeCompare(b.label), visibility: isRawMode, }; @@ -276,6 +278,8 @@ const config: ControlPanelConfig = { choices: datasource?.order_by_choices || [], }), visibility: isRawMode, + sortComparator: (a: { label: string }, b: { label: string }) => + a.label.localeCompare(b.label), }, }, ], diff --git a/superset-frontend/spec/javascripts/explore/components/SelectControl_spec.jsx b/superset-frontend/spec/javascripts/explore/components/SelectControl_spec.jsx index bc16485dab..c2a69a2631 100644 --- a/superset-frontend/spec/javascripts/explore/components/SelectControl_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/SelectControl_spec.jsx @@ -27,6 +27,7 @@ import { styledMount as mount } from 'spec/helpers/theming'; const defaultProps = { choices: [ ['1 year ago', '1 year ago'], + ['1 week ago', '1 week ago'], ['today', 'today'], ], name: 'row_limit', @@ -36,8 +37,9 @@ const defaultProps = { }; const options = [ - { value: '1 year ago', label: '1 year ago' }, - { value: 'today', label: 'today' }, + { value: '1 year ago', label: '1 year ago', order: 0 }, + { value: '1 week ago', label: '1 week ago', order: 1 }, + { value: 'today', label: 'today', order: 2 }, ]; describe('SelectControl', () => { @@ -149,6 +151,37 @@ describe('SelectControl', () => { expect(wrapper.html()).not.toContain('add something'); }); }); + + describe('when select has a sortComparator prop', () => { + it('does not add add order key and sorts by sortComparator', () => { + const sortComparator = (a, b) => a.label.localeCompare(b.label); + const optionsSortedByLabel = options + .map(opt => ({ label: opt.label, value: opt.value })) + .sort(sortComparator); + wrapper = mount( + , + ); + expect(wrapper.state().options).toEqual(optionsSortedByLabel); + }); + }); + + describe('when select does not have a sortComparator prop', () => { + it('adds an order key and maintains its intial order', () => { + wrapper = mount( + , + ); + expect(wrapper.state().options).toEqual(options); + }); + }); }); describe('getOptions', () => { @@ -160,8 +193,8 @@ describe('SelectControl', () => { describe('UNSAFE_componentWillReceiveProps', () => { it('sets state.options if props.choices has changed', () => { const updatedOptions = [ - { value: 'three', label: 'three' }, - { value: 'four', label: 'four' }, + { value: 'three', label: 'three', order: 0 }, + { value: 'four', label: 'four', order: 1 }, ]; const newProps = { choices: [ diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx index 1afca44663..c5d41636f8 100644 --- a/superset-frontend/src/components/Select/Select.tsx +++ b/superset-frontend/src/components/Select/Select.tsx @@ -171,7 +171,6 @@ const StyledSelect = styled(AntdSelect)` && .ant-select-selector { border-radius: ${theme.gridUnit}px; } - // Open the dropdown when clicking on the suffix // This is fixed in version 4.16 .ant-select-arrow .anticon:not(.ant-select-suffix) { @@ -196,7 +195,6 @@ const StyledError = styled.div` width: 100%; padding: ${theme.gridUnit * 2}px; color: ${theme.colors.error.base}; - & svg { margin-right: ${theme.gridUnit * 2}px; } @@ -298,8 +296,9 @@ const Select = ({ const shouldShowSearch = isAsync || allowNewOptions ? true : showSearch; const initialOptions = options && Array.isArray(options) ? options : EMPTY_OPTIONS; - const [selectOptions, setSelectOptions] = - useState(initialOptions); + const [selectOptions, setSelectOptions] = useState( + initialOptions.sort(sortComparator), + ); const shouldUseChildrenOptions = !!selectOptions.find( opt => opt?.customLabel, ); diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx index c9fe9c5ba9..0f037fb31d 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx @@ -37,6 +37,7 @@ import AdhocFilter, { CLAUSES, } from 'src/explore/components/controls/FilterControl/AdhocFilter'; import { Input } from 'src/common/components'; +import { propertyComparator } from 'src/components/Select/Select'; const StyledInput = styled(Input)` margin-bottom: ${({ theme }) => theme.gridUnit * 4}px; @@ -387,12 +388,14 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC = props => { css={theme => ({ marginBottom: theme.gridUnit * 4 })} options={(props.operators ?? OPERATORS_OPTIONS) .filter(op => isOperatorRelevant(op, subject)) - .map(option => ({ + .map((option, index) => ({ value: option, label: OPERATOR_ENUM_TO_OPERATOR_TYPE[option].display, key: option, + order: index, }))} {...operatorSelectProps} + sortComparator={propertyComparator('order')} /> {MULTI_OPERATORS.has(operatorId) || suggestions.length > 0 ? ( = props => { label: String(suggestion), }))} {...comparatorSelectProps} + sortComparator={propertyComparator( + typeof suggestions[0] === 'number' ? 'value' : 'label', + )} /> ) : ( { + options = choices.map((c, i) => { if (Array.isArray(c)) { const [value, label] = c.length > 1 ? c : [c[0], c[0]]; + if (!this.props.sortComparator) return { value, label, order: i }; return { value, label, @@ -240,6 +241,7 @@ export default class SelectControl extends React.PureComponent { optionRenderer, options: this.state.options, placeholder, + sortComparator: this.props.sortComparator || propertyComparator('order'), value: getValue(), };