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.

Sign up for GitHub

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

Jump to bottom

fix(select): fix typing for customLabel #18952

Closed
wants to merge 2 commits into from
Closed

fix(select): fix typing for customLabel #18952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d453ffd
fix(select): fix typing for customLabel
ktmud Feb 25, 2022
75908ca
Dont export SelectValue
ktmud Feb 28, 2022
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
9 changes: 3 additions & 6 deletions superset-frontend/src/addSlice/AddSliceContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { ReactNode } from 'react';
import React from 'react';
import rison from 'rison';
import { styled, t, SupersetClient, JsonResponse } from '@superset-ui/core';
import { Steps } from 'src/common/components';
import Button from 'src/components/Button';
import { Select } from 'src/components';
import { FormLabel } from 'src/components/Form';
import { Tooltip } from 'src/components/Tooltip';
import { LabeledValues } from 'src/components/Select/Select';

import VizTypeGallery, {
MAX_ADVISABLE_VIZ_GALLERY_WIDTH,
Expand Down Expand Up @@ -252,11 +253,7 @@ export default class AddSliceContainer extends React.PureComponent<
return SupersetClient.get({
endpoint: `/api/v1/dataset/?q=${query}`,
}).then((response: JsonResponse) => {
const list: {
customLabel: ReactNode;
label: string;
value: string;
}[] = response.json.result.map((item: Dataset) => ({
const list: LabeledValues = response.json.result.map((item: Dataset) => ({
value: `${item.id}__${item.datasource_type}`,
customLabel: this.newLabel(item),
label: item.table_name,
Expand Down
70 changes: 37 additions & 33 deletions superset-frontend/src/components/Select/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import { styled, t } from '@superset-ui/core';
import AntdSelect, {
SelectProps as AntdSelectProps,
SelectValue as AntdSelectValue,
LabeledValue as AntdLabeledValue,
} from 'antd/lib/select';
import { DownOutlined, SearchOutlined } from '@ant-design/icons';
import debounce from 'lodash/debounce';
Expand All @@ -44,7 +43,21 @@ import { hasOption } from './utils';

const { Option } = AntdSelect;

type AntdSelectAllProps = AntdSelectProps<AntdSelectValue>;
export type RawValue = string | number;

export interface LabeledValue {
key?: string;
value: RawValue;
label: ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

The idea when introducing the customLabel prop was that label would always be a string that could be searched/compared and customLabel would be used when you want a React.Node. The hasOption function is comparing the labels as strings. One example of this use is the tables select where label is the name of the table, customLabel is a node with an icon and the name, and value is the id of the table.

Copy link
Member Author

@ktmud ktmud Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I understand the intention but changing it to ReactNode will cause too many incompatibility in other references of the Select component. Ideally we should probably refactor customLabel to something akin to optionalRenderer. It also doesn't feel right for a data fetching API (the options promise) to return ReactNode. Data fetching and rendering should be two separate things.

customLabel?: ReactNode;
}

export type RawValues = RawValue[];
export type LabeledValues = LabeledValue[];
export type OptionsType = LabeledValues;

type SelectValue = RawValue | LabeledValue | RawValues | LabeledValues;
type AntdSelectAllProps = AntdSelectProps<SelectValue>;

type PickedSelectProps = Pick<
AntdSelectAllProps,
Expand All @@ -63,15 +76,8 @@ type PickedSelectProps = Pick<
| 'value'
>;

type OptionsProps = Exclude<AntdSelectAllProps['options'], undefined>;

export interface OptionsType extends Omit<OptionsProps, 'label'> {
label?: string;
customLabel?: ReactNode;
}

export type OptionsTypePage = {
data: OptionsType;
data: LabeledValues;
totalCount: number;
};

Expand Down Expand Up @@ -157,7 +163,7 @@ export interface SelectProps extends PickedSelectProps {
* Works in async mode only (See the options property).
*/
onError?: (error: string) => void;
sortComparator?: (a: AntdLabeledValue, b: AntdLabeledValue) => number;
sortComparator?: (a: LabeledValue, b: LabeledValue) => number;
}

const StyledContainer = styled.div`
Expand Down Expand Up @@ -230,7 +236,7 @@ const Error = ({ error }: { error: string }) => (
</StyledError>
);

const defaultSortComparator = (a: AntdLabeledValue, b: AntdLabeledValue) => {
const defaultSortComparator = (a: LabeledValue, b: LabeledValue) => {
if (typeof a.label === 'string' && typeof b.label === 'string') {
return a.label.localeCompare(b.label);
}
Expand All @@ -245,7 +251,7 @@ const defaultSortComparator = (a: AntdLabeledValue, b: AntdLabeledValue) => {
* Can be used with string and number property values.
* */
export const propertyComparator =
(property: string) => (a: AntdLabeledValue, b: AntdLabeledValue) => {
(property: string) => (a: LabeledValue, b: LabeledValue) => {
if (typeof a[property] === 'string' && typeof b[property] === 'string') {
return a[property].localeCompare(b[property]);
}
Expand Down Expand Up @@ -333,15 +339,15 @@ const Select = ({
if (Array.isArray(selectedValue)) {
if (isLabeledValue) {
found =
(selectedValue as AntdLabeledValue[]).find(
(selectedValue as LabeledValue[]).find(
element => element.value === opt.value,
) !== undefined;
} else {
found = selectedValue.includes(opt.value);
found = (selectedValue as RawValue[]).includes(opt.value);
}
} else {
found = isLabeledValue
? (selectedValue as AntdLabeledValue).value === opt.value
? (selectedValue as LabeledValue).value === opt.value
: selectedValue === opt.value;
}

Expand All @@ -355,16 +361,16 @@ const Select = ({
// fallback for custom options in tags mode as they
// do not appear in the selectOptions state
if (!isSingleMode && Array.isArray(selectedValue)) {
selectedValue.forEach((val: string | number | AntdLabeledValue) => {
selectedValue.forEach((val: string | number | LabeledValue) => {
if (
!topOptions.find(
tOpt =>
tOpt.value ===
(isLabeledValue ? (val as AntdLabeledValue)?.value : val),
(isLabeledValue ? (val as LabeledValue)?.value : val),
)
) {
if (isLabeledValue) {
const labelValue = val as AntdLabeledValue;
const labelValue = val as LabeledValue;
topOptions.push({
label: labelValue.label,
value: labelValue.value,
Expand Down Expand Up @@ -393,9 +399,7 @@ const Select = ({
[isAsync, isSingleMode, labelInValue, selectOptions, sortComparator],
);

const handleOnSelect = (
selectedValue: string | number | AntdLabeledValue,
) => {
const handleOnSelect = (selectedValue: string | number | LabeledValue) => {
if (isSingleMode) {
setSelectValue(selectedValue);
} else {
Expand All @@ -414,21 +418,21 @@ const Select = ({
]);
} else {
setSelectValue([
...(currentSelected as AntdLabeledValue[]),
selectedValue as AntdLabeledValue,
...(currentSelected as LabeledValue[]),
selectedValue as LabeledValue,
]);
}
}
setSearchedValue('');
};

const handleOnDeselect = (value: string | number | AntdLabeledValue) => {
const handleOnDeselect = (value: string | number | LabeledValue) => {
if (Array.isArray(selectValue)) {
if (typeof value === 'number' || typeof value === 'string') {
const array = selectValue as (string | number)[];
setSelectValue(array.filter(element => element !== value));
} else {
const array = selectValue as AntdLabeledValue[];
const array = selectValue as LabeledValue[];
setSelectValue(array.filter(element => element.value !== value.value));
}
}
Expand Down Expand Up @@ -557,7 +561,7 @@ const Select = ({
}
};

const handleFilterOption = (search: string, option: AntdLabeledValue) => {
const handleFilterOption = (search: string, option: LabeledValue) => {
if (typeof filterOption === 'function') {
return filterOption(search, option);
}
Expand Down Expand Up @@ -643,22 +647,22 @@ const Select = ({
if (selectValue) {
setSelectOptions(selectOptions => {
const array = Array.isArray(selectValue)
? (selectValue as AntdLabeledValue[])
: [selectValue as AntdLabeledValue | string | number];
const options: AntdLabeledValue[] = [];
? (selectValue as LabeledValue[])
: [selectValue as LabeledValue | string | number];
const options: LabeledValue[] = [];
const isLabeledValue = isAsync || labelInValue;
array.forEach(element => {
const found = selectOptions.find(
(option: { value: string | number }) =>
isLabeledValue
? option.value === (element as AntdLabeledValue).value
? option.value === (element as LabeledValue).value
: option.value === element,
);
if (!found) {
options.push(
isLabeledValue
? (element as AntdLabeledValue)
: ({ value: element, label: element } as AntdLabeledValue),
? (element as LabeledValue)
: ({ value: element, label: element } as LabeledValue),
);
}
});
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/components/TableSelector/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
setLoadingTables(false);
if (forceRefresh) addSuccessToast('List updated');
})
.catch(e => {
.catch(() => {
setLoadingTables(false);
handleError(t('There was an error loading the tables'));
});
Expand Down
2 changes: 1 addition & 1 deletion ...xplore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC<Props> = props => {
('column_name' in column && column.column_name) ||
('label' in column && column.label),
key:
('id' in column && column.id) ||
('id' in column && String(column.id)) ||
('optionName' in column && column.optionName) ||
undefined,
customLabel: renderSubjectOptionLabel(column),
Expand Down