From a38535b5dbb5a991d807141b9205d8ab703d675e Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Thu, 2 Dec 2021 15:18:08 -0700 Subject: [PATCH 1/6] fix: Select component sort function sorting by label instead of value on numbers --- .../src/shared-controls/index.tsx | 3 +++ .../src/components/Select/Select.tsx | 27 +++++++++++-------- .../components/RefreshIntervalModal.tsx | 4 +-- .../FormattingPopoverContent.tsx | 4 +-- .../DateFilterControl/DateFilterLabel.tsx | 4 +-- .../components/CustomFrame.tsx | 12 ++++----- .../components/controls/SelectControl.jsx | 1 + .../src/views/CRUD/alert/AlertReportModal.tsx | 6 ++--- 8 files changed, 34 insertions(+), 27 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 6017d50b99e52..1bf71a7d24743 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 @@ -333,6 +333,7 @@ const row_limit: SharedControlConfig<'SelectControl'> = { default: 10000, choices: formatSelectOptions(ROW_LIMIT_OPTIONS), description: t('Limits the number of rows that get displayed.'), + sortByProperty: 'value', }; const limit: SharedControlConfig<'SelectControl'> = { @@ -348,6 +349,7 @@ const limit: SharedControlConfig<'SelectControl'> = { 'fetched and rendered. This feature is useful when grouping by high cardinality ' + 'column(s) though does increase the query complexity and cost.', ), + sortByProperty: 'value', }; const series_limit: SharedControlConfig<'SelectControl'> = { @@ -362,6 +364,7 @@ const series_limit: SharedControlConfig<'SelectControl'> = { 'fetched and rendered. This feature is useful when grouping by high cardinality ' + 'column(s) though does increase the query complexity and cost.', ), + sortByProperty: 'value', }; const sort_by: SharedControlConfig<'MetricsControl'> = { diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx index 1afca4466336f..a1b61ac106b4b 100644 --- a/superset-frontend/src/components/Select/Select.tsx +++ b/superset-frontend/src/components/Select/Select.tsx @@ -157,6 +157,7 @@ export interface SelectProps extends PickedSelectProps { * Works in async mode only (See the options property). */ onError?: (error: string) => void; + sortByProperty?: string; sortComparator?: (a: AntdLabeledValue, b: AntdLabeledValue) => number; } @@ -232,15 +233,19 @@ const Error = ({ error }: { error: string }) => ( ); -const defaultSortComparator = (a: AntdLabeledValue, b: AntdLabeledValue) => { - if (typeof a.label === 'string' && typeof b.label === 'string') { - return a.label.localeCompare(b.label); - } - if (typeof a.value === 'string' && typeof b.value === 'string') { - return a.value.localeCompare(b.value); - } - return (a.value as number) - (b.value as number); -}; +// Not sure if this is necessary anymore? Seems like all options being +// passed in are being formatted to have a string label +// ----------------------------------------- +// const defaultSortComparator = (a: AntdLabeledValue, b: AntdLabeledValue) => { +// if (typeof a.label === 'string' && typeof b.label === 'string') { +// return a.label.localeCompare(b.label); +// } +// if (typeof a.value === 'string' && typeof b.value === 'string') { +// return a.value.localeCompare(b.value); +// } +// return (a.value as number) - (b.value as number); +// }; +// ----------------------------------------- /** * It creates a comparator to check for a specific property. @@ -289,7 +294,8 @@ const Select = ({ pageSize = DEFAULT_PAGE_SIZE, placeholder = t('Select ...'), showSearch = true, - sortComparator = defaultSortComparator, + sortByProperty = 'label', + sortComparator = propertyComparator(sortByProperty), value, ...props }: SelectProps) => { @@ -328,7 +334,6 @@ const Select = ({ const isLabeledValue = isAsync || labelInValue; const topOptions: OptionsType = []; const otherOptions: OptionsType = []; - selectOptions.forEach(opt => { let found = false; if (Array.isArray(selectedValue)) { diff --git a/superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx b/superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx index 3a8aef03727c9..b2d26b84ac73f 100644 --- a/superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx +++ b/superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx @@ -17,7 +17,7 @@ * under the License. */ import React, { RefObject } from 'react'; -import Select, { propertyComparator } from 'src/components/Select/Select'; +import Select from 'src/components/Select/Select'; import { t, styled } from '@superset-ui/core'; import Alert from 'src/components/Alert'; import Button from 'src/components/Button'; @@ -120,7 +120,7 @@ class RefreshIntervalModal extends React.PureComponent< options={options} value={refreshFrequency} onChange={this.handleFrequencyChange} - sortComparator={propertyComparator('value')} + sortByProperty="value" /> {showRefreshWarning && ( diff --git a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx index ee862b2b69732..d72b359712265 100644 --- a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx +++ b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx @@ -19,7 +19,7 @@ import React from 'react'; import { styled, t } from '@superset-ui/core'; import { Form, FormItem, FormProps } from 'src/components/Form'; -import Select, { propertyComparator } from 'src/components/Select/Select'; +import Select from 'src/components/Select/Select'; import { Col, InputNumber, Row } from 'src/common/components'; import Button from 'src/components/Button'; import { @@ -129,7 +129,7 @@ const operatorField = ( ); diff --git a/superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx b/superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx index 887a6c5075609..e5324a926bdd3 100644 --- a/superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx +++ b/superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx @@ -40,7 +40,7 @@ import Label, { Type } from 'src/components/Label'; import Popover from 'src/components/Popover'; import { Divider } from 'src/common/components'; import Icons from 'src/components/Icons'; -import Select from 'src/components/Select/Select'; +import Select, { propertyComparator } from 'src/components/Select/Select'; import { Tooltip } from 'src/components/Tooltip'; import { DEFAULT_TIME_RANGE } from 'src/explore/constants'; import { useDebouncedEffect } from 'src/explore/exploreUtils'; @@ -295,8 +295,7 @@ export default function DateFilterLabel(props: DateFilterControlProps) { options={FRAME_OPTIONS} value={frame} onChange={onChangeFrame} - sortByProperty="order" - sortOptions + sortComparator={propertyComparator('order')} /> {frame !== 'No filter' && } {frame === 'Common' && ( diff --git a/superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx b/superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx index baffb6ac44a46..a26d7ca765574 100644 --- a/superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx +++ b/superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx @@ -23,7 +23,7 @@ import { isInteger } from 'lodash'; import { Col, InputNumber, Row } from 'src/common/components'; import { DatePicker } from 'src/components/DatePicker'; import { Radio } from 'src/components/Radio'; -import Select from 'src/components/Select/Select'; +import Select, { propertyComparator } from 'src/components/Select/Select'; import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls'; import { SINCE_GRAIN_OPTIONS, @@ -41,6 +41,8 @@ import { FrameComponentProps, } from 'src/explore/components/controls/DateFilterControl/types'; +const sortComparator = propertyComparator('order'); + export function CustomFrame(props: FrameComponentProps) { const { customRange, matchedFlag } = customTimeRangeDecode(props.value); if (!matchedFlag) { @@ -121,8 +123,7 @@ export function CustomFrame(props: FrameComponentProps) { options={SINCE_MODE_OPTIONS} value={sinceMode} onChange={(value: string) => onChange('sinceMode', value)} - sortByProperty="order" - sortOptions + sortComparator={sortComparator} /> {sinceMode === 'specific' && ( @@ -157,8 +158,7 @@ export function CustomFrame(props: FrameComponentProps) { options={SINCE_GRAIN_OPTIONS} value={sinceGrain} onChange={(value: string) => onChange('sinceGrain', value)} - sortByProperty="order" - sortOptions + sortComparator={sortComparator} /> @@ -177,8 +177,7 @@ export function CustomFrame(props: FrameComponentProps) { options={UNTIL_MODE_OPTIONS} value={untilMode} onChange={(value: string) => onChange('untilMode', value)} - sortByProperty="order" - sortOptions + sortComparator={sortComparator} /> {untilMode === 'specific' && ( @@ -212,8 +211,7 @@ export function CustomFrame(props: FrameComponentProps) { options={UNTIL_GRAIN_OPTIONS} value={untilGrain} onChange={(value: string) => onChange('untilGrain', value)} - sortByProperty="order" - sortOptions + sortComparator={sortComparator} /> 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 c9fe9c5ba9ee1..0f037fb31d945 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,8 +241,7 @@ export default class SelectControl extends React.PureComponent { optionRenderer, options: this.state.options, placeholder, - sortByProperty: this.props.sortByProperty, - sortOptions: this.props.sortOptions, + sortComparator: this.props.sortComparator || propertyComparator('order'), value: getValue(), }; diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx index ee233e8acc670..99deaa8cc1fdb 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx +++ b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx @@ -38,7 +38,7 @@ import { Switch } from 'src/components/Switch'; import Modal from 'src/components/Modal'; import TimezoneSelector from 'src/components/TimezoneSelector'; import { Radio } from 'src/components/Radio'; -import Select from 'src/components/Select/Select'; +import Select, { propertyComparator } from 'src/components/Select/Select'; import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; import withToasts from 'src/components/MessageToasts/withToasts'; import Owner from 'src/types/Owner'; @@ -1177,8 +1177,7 @@ const AlertReportModal: FunctionComponent = ({ currentAlert?.validator_config_json?.op || undefined } options={CONDITIONS} - sortByProperty="order" - sortOptions + sortComparator={propertyComparator('order')} /> @@ -1250,8 +1249,7 @@ const AlertReportModal: FunctionComponent = ({ : DEFAULT_RETENTION } options={RETENTION_OPTIONS} - sortByProperty="value" - sortOptions + sortComparator={propertyComparator('value')} /> From 47824735947cb2bc67de39e918052ffef5194d78 Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Tue, 7 Dec 2021 18:19:17 -0700 Subject: [PATCH 5/6] fix: select component options intitial sort bug --- superset-frontend/src/components/Select/Select.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx index 6d90d18bedab5..c5d41636f82ae 100644 --- a/superset-frontend/src/components/Select/Select.tsx +++ b/superset-frontend/src/components/Select/Select.tsx @@ -296,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, ); From 499ae7febc61fbfc5939f76f6a716d338843df38 Mon Sep 17 00:00:00 2001 From: Corbin Robb Date: Wed, 8 Dec 2021 13:15:11 -0700 Subject: [PATCH 6/6] fix: add test cases for select fix --- .../explore/components/SelectControl_spec.jsx | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/superset-frontend/spec/javascripts/explore/components/SelectControl_spec.jsx b/superset-frontend/spec/javascripts/explore/components/SelectControl_spec.jsx index e43ca4db9bed1..c2a69a2631eb1 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', @@ -37,7 +38,8 @@ const defaultProps = { const options = [ { value: '1 year ago', label: '1 year ago', order: 0 }, - { value: 'today', label: 'today', order: 1 }, + { 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', () => {