EnxDev commented on code in PR #45168:
URL: https://github.com/apache/superset/pull/45168#discussion_r4256175469
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -101,6 +101,11 @@ const Error = ({ error }: { error: string }) => (
const getQueryCacheKey = (value: string, page: number, pageSize: number) =>
`${value};${page};${pageSize}`;
+const normalizeOptionLabel = (label: unknown): string | undefined =>
+ typeof label === 'string' || typeof label === 'number'
+ ? String(label).trim().toLowerCase()
Review Comment:
This applies to every `AsyncSelect` with `allowNewOptions`, not only the
dashboard picker. Dashboard titles aren't unique or case-insensitive on the
backend, so once the server returns "Test Dashboard" a user can no longer
create "test dashboard", and the same rule now holds for the recipients select
in `NotificationMethod`.
Is that what we want for all callers? A prop, or doing the dedupe in
`SaveModal`, would keep it scoped to the case in the issue.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -570,19 +590,34 @@ const AsyncSelect = forwardRef(
if (allowNewOptions) {
const unquotedSearch = stripSurroundingQuotes(searchValue);
- const newOption = unquotedSearch &&
- !hasOption(unquotedSearch, fullSelectOptions, true) && {
- label: unquotedSearch,
- value: unquotedSearch,
- isNewOption: true,
- };
- const cleanSelectOptions = fullSelectOptions.filter(
- opt => !opt.isNewOption || hasOption(opt.value, selectValue),
- );
- const newOptions = newOption
- ? [newOption, ...cleanSelectOptions]
- : cleanSelectOptions;
- setSelectOptions(newOptions);
+ setSelectOptions(prevOptions => {
Review Comment:
The old code checked the typed text against `fullSelectOptions`, which
includes selected values that aren't in the loaded page. `prevOptions` doesn't,
so in a multi select, typing the label of something already selected but not
loaded looks like it would be offered again as a new option.
Am I reading that right? If so, folding the missing selected values into the
check would keep the old behaviour.
--
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]
