You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/03/21 15:48:30 UTC
[GitHub] [superset] ktmud commented on a change in pull request #19283: fix: Don't allow duplicated tag values in the Select
ktmud commented on a change in pull request #19283:
URL: https://github.com/apache/superset/pull/19283#discussion_r831254519
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -385,31 +385,37 @@ const Select = (
const hasCustomLabels = fullSelectOptions.some(opt => !!opt?.customLabel);
+ const valueAsArray = <T,>(value: any): T[] => {
+ const array = value ? (Array.isArray(value) ? value : [value]) : [];
+ return array as T[];
+ };
+
const handleOnSelect = (
selectedValue: string | number | AntdLabeledValue,
) => {
if (isSingleMode) {
setSelectValue(selectedValue);
+ } else if (
+ typeof selectedValue === 'number' ||
+ typeof selectedValue === 'string'
+ ) {
+ setSelectValue(previousState => {
+ const primitiveArray = valueAsArray<string | number>(previousState);
+ // Tokenized values can contain duplicated values
+ if (!primitiveArray.some(value => value === selectedValue)) {
+ return [...primitiveArray, selectedValue as string | number];
+ }
+ return previousState;
+ });
} else {
- const currentSelected = selectValue
- ? Array.isArray(selectValue)
- ? selectValue
- : [selectValue]
- : [];
- if (
- typeof selectedValue === 'number' ||
- typeof selectedValue === 'string'
- ) {
- setSelectValue([
- ...(currentSelected as (string | number)[]),
- selectedValue as string | number,
- ]);
- } else {
- setSelectValue([
- ...(currentSelected as AntdLabeledValue[]),
- selectedValue as AntdLabeledValue,
- ]);
- }
+ setSelectValue(previousState => {
+ const objectArray = valueAsArray<AntdLabeledValue>(previousState);
+ // Tokenized values can contain duplicated values
+ if (!objectArray.some(object => object.value === selectedValue.label)) {
Review comment:
Why are we comparing value to labels?
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -385,31 +385,37 @@ const Select = (
const hasCustomLabels = fullSelectOptions.some(opt => !!opt?.customLabel);
+ const valueAsArray = <T,>(value: any): T[] => {
+ const array = value ? (Array.isArray(value) ? value : [value]) : [];
+ return array as T[];
+ };
Review comment:
There is a `ensureIsArray` function in `@superset-ui/core`
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -385,31 +385,37 @@ const Select = (
const hasCustomLabels = fullSelectOptions.some(opt => !!opt?.customLabel);
+ const valueAsArray = <T,>(value: any): T[] => {
+ const array = value ? (Array.isArray(value) ? value : [value]) : [];
+ return array as T[];
+ };
+
const handleOnSelect = (
selectedValue: string | number | AntdLabeledValue,
) => {
if (isSingleMode) {
setSelectValue(selectedValue);
+ } else if (
+ typeof selectedValue === 'number' ||
+ typeof selectedValue === 'string'
+ ) {
+ setSelectValue(previousState => {
+ const primitiveArray = valueAsArray<string | number>(previousState);
+ // Tokenized values can contain duplicated values
+ if (!primitiveArray.some(value => value === selectedValue)) {
Review comment:
```suggestion
if (!primitiveArray.includes(selectedValue)) {
```
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -385,31 +385,37 @@ const Select = (
const hasCustomLabels = fullSelectOptions.some(opt => !!opt?.customLabel);
+ const valueAsArray = <T,>(value: any): T[] => {
+ const array = value ? (Array.isArray(value) ? value : [value]) : [];
+ return array as T[];
+ };
+
const handleOnSelect = (
selectedValue: string | number | AntdLabeledValue,
) => {
if (isSingleMode) {
setSelectValue(selectedValue);
+ } else if (
+ typeof selectedValue === 'number' ||
+ typeof selectedValue === 'string'
+ ) {
Review comment:
I believe these two if/else can be rewritten with `hasOption` in Select/utils.
##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -385,31 +385,37 @@ const Select = (
const hasCustomLabels = fullSelectOptions.some(opt => !!opt?.customLabel);
+ const valueAsArray = <T,>(value: any): T[] => {
+ const array = value ? (Array.isArray(value) ? value : [value]) : [];
+ return array as T[];
+ };
+
const handleOnSelect = (
selectedValue: string | number | AntdLabeledValue,
Review comment:
Can we rename this local `selectedValue` to `selectedItem` so not to confuse with `selectValue` in the parent context which may also be an array?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org