sadpandajoe commented on code in PR #45591:
URL: https://github.com/apache/superset/pull/45591#discussion_r4368330923
##########
superset-frontend/src/features/groups/utils.ts:
##########
@@ -18,10 +18,80 @@
*/
import { t } from '@apache-superset/core/translation';
import { SupersetClient } from '@superset-ui/core';
+import {
+ DEFAULT_SORT_COMPARATOR,
+ type LabeledValue,
+ stripSurroundingQuotes,
+} from '@superset-ui/core/components';
import rison from 'rison';
+import {
+ SUBJECT_DETAIL_PROP,
+ SUBJECT_OPTION_FILTER_PROPS,
+ SUBJECT_TEXT_LABEL_PROP,
+ SubjectSelectLabel,
+} from 'src/features/subjects/SubjectSelectLabel';
import { getUserDisplayLabel } from 'src/features/users/utils';
import { FormValues } from './types';
+type UserRecord = { id: number; email?: string } & Parameters<
+ typeof getUserDisplayLabel
+>[0];
+
+/**
+ * Option props AsyncSelect's client-side re-filter should match against for
+ * user options: the display name, the email and the username. These mirror
+ * the columns the server-side `user_all_text` filter searches, so a user the
+ * server matched is never hidden by the picker.
+ */
+export const USER_OPTION_FILTER_PROPS = [
+ ...SUBJECT_OPTION_FILTER_PROPS,
+ 'username',
+];
+
+/**
+ * Build a select option for a user picker.
+ *
+ * The option shows the email under the display name, like the subject-based
+ * owner pickers do, so that a server-side match on email is visible to the
+ * person searching. Plain-text copies of both lines, plus the username, are
+ * attached under the `USER_OPTION_FILTER_PROPS` keys so AsyncSelect's
+ * client-side re-filter can match them (a React label is otherwise filtered
+ * as an empty string), and the display name under `title` so ListView can
+ * serialize the chosen option's label into the URL. The display name is also
+ * the option's `aria-label`, since the virtualized option list only derives
+ * one from a string label and would otherwise announce the numeric user id.
+ * The username is not rendered, so a username match surfaces the user
+ * without explaining why.
+ */
+export const toUserSelectOption = (user: UserRecord) => {
+ const textLabel = getUserDisplayLabel(user);
+ return {
+ value: user.id,
+ label: SubjectSelectLabel({ label: textLabel, secondaryLabel: user.email
}),
+ title: textLabel,
+ 'aria-label': textLabel,
Review Comment:
When the edit modal is opened for a group or role whose member isn't in the
first page of fetched users, `AsyncSelect` rebuilds that selected option from
only `{ value, label }` (the `fullSelectOptions` fallback), so the `aria-label`
set here is dropped. The label is now JSX rather than a string, so the
virtualized option list has no accessible name and screen readers announce the
numeric user id. Should that fallback keep the rest of the option
(aria-label/title/filter props), and could the Edit modal test seed a
preselected user outside the mocked first page and assert an option named by
display name (not the id) is exposed?
--
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]
