michael-s-molina commented on code in PR #32514:
URL: https://github.com/apache/superset/pull/32514#discussion_r1998874760
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -165,16 +172,21 @@ const Select = forwardRef(
sortSelectedFirst,
sortComparator,
),
- [inputValue, sortComparator, sortSelectedFirst],
+ [inputValue, sortComparator, isDropdownVisible],
);
const initialOptions = useMemo(
() => (Array.isArray(options) ? options.slice() : EMPTY_OPTIONS),
[options],
);
const initialOptionsSorted = useMemo(
- () => initialOptions.slice().sort(sortSelectedFirst),
- [initialOptions, sortSelectedFirst],
+ () =>
+ [...initialOptions].sort((a, b) => {
Review Comment:
What was wrong with the previous implementation? It seems we changed the
sorting order here.
##########
superset-frontend/src/components/Select/AsyncSelect.tsx:
##########
@@ -219,7 +222,9 @@ const AsyncSelect = forwardRef(
const missingValues: SelectOptionsType = ensureIsArray(selectValue)
.filter(opt => !hasOption(getValue(opt), selectOptions))
.map(opt =>
- isLabeledValue(opt) ? opt : { value: opt, label: String(opt) },
+ isLabeledValue(opt)
+ ? { value: opt.value, label: opt.label }
Review Comment:
If it's already a `LabeledValue`, why do we need to create a new object?
##########
superset-frontend/src/components/Select/AsyncSelect.tsx:
##########
@@ -197,7 +200,7 @@ const AsyncSelect = forwardRef(
sortSelectedFirst,
sortComparator,
),
- [inputValue, sortComparator, sortSelectedFirst],
+ [inputValue, sortComparator, isDropdownVisible],
Review Comment:
Why this change given that `sortSelectedFirst` is referenced by the
`sortComparatorWithSearch` and `isDropdownVisible` is not?
##########
superset-frontend/src/components/Select/Select.test.tsx:
##########
@@ -110,6 +110,13 @@ const findSelectValue = () =>
const findAllSelectValues = () =>
waitFor(() => [...getElementsByClassName('.ant-select-selection-item')]);
+// Tag sets classname manually.
+const findTagSelectValue = () =>
+ waitFor(() => getElementByClassName('.antd5-select-selection-item'));
+
+const findAllTagSelectValues = () =>
Review Comment:
Why do we have a difference now in the tests between select value and tag
select value?
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -264,8 +276,12 @@ const Select = forwardRef(
}
return [
SELECT_ALL_VALUE,
- ...selectAllEligible.map(opt => opt.value),
- ] as AntdLabeledValue[];
+ ...selectAllEligible
+ .map(opt => opt.value)
+ .filter(
+ (val): val is RawValue => val !== null && val !== undefined,
Review Comment:
Yes, `null` values are acceptable.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
