Updated pickers with input attribute compatibility. #33243

tpalacino · 2024-11-10T17:06:20Z

Previous Behavior

The Picker components that extend the BasePicker and BasePickerListBelow are missing properties that most other FluentUI user input component support. The missing properties include label, required, errorMessage, and onGetErrorMessage.

New Behavior

The pickers now support the following optional properties:

Property	Description
label	When specified renders consistently with how a label is rendered for a `TextField`.
required	When specified renders consistently with how the required attribute is rendered for a `TextField`.
errorMessage	When specified renders consistently with how an error message is rendered for a `ComboBox`.
onGetErrorMessage	When specified behaves consistently with how an error message function behaves for a `TextField`.

Related Issue(s)

Fixes NormalPeoplePicker required attribute #22528

tpalacino · 2024-11-16T13:51:34Z

@gouttierre @msft-fluent-ui-bot @JustSlone @layershifter @khmakoto @micahgodbolt Can someone please help me merge this?

smhigley

Thanks for doing this work! I did an initial pass and left comments on stuff I saw. Let me know if they make sense, and if you have any questions!

packages/react/src/components/pickers/BasePicker.tsx

packages/react/src/components/pickers/BasePicker.types.ts

packages/react/src/components/pickers/BasePicker.tsx

change/@fluentui-react-fd94592f-6c11-434f-a340-fce340db9a34.json

tpalacino · 2024-11-19T21:14:11Z

@smhigley & @khmakoto Feedback has been resolved

packages/react/src/components/pickers/BasePicker.types.ts

packages/react-examples/src/react/PeoplePicker/PeoplePicker.Normal.Example.tsx

packages/react-examples/src/react/PeoplePicker/PeoplePicker.List.Example.tsx

packages/react-examples/src/react/PeoplePicker/PeoplePicker.Normal.Example.tsx

packages/react-examples/src/react/PeoplePicker/PeoplePicker.List.Example.tsx

khmakoto · 2024-11-21T22:05:09Z

packages/react/src/components/pickers/BasePicker.tsx

+          error: css('ms-BasePicker-error', ''),
+          label: css('ms-BasePicker-label', ''),


If you're not passing anything else here, you can just do:

Suggested change

error: css('ms-BasePicker-error', ''),

label: css('ms-BasePicker-label', ''),

error: 'ms-BasePicker-error',

label: 'ms-BasePicker-label',

khmakoto · 2024-11-21T22:05:37Z

packages/react/src/components/pickers/BasePicker.tsx

          text: css('ms-BasePicker-text', legacyStyles.pickerText, this.state.isFocused && legacyStyles.inputFocused),
          itemsWrapper: legacyStyles.pickerItems,
          input: css('ms-BasePicker-input', legacyStyles.pickerInput, inputProps && inputProps.className),
          screenReaderText: legacyStyles.screenReaderOnly,
        };

    const comboLabel = this.props['aria-label'] || inputProps?.['aria-label'];
+    const inputId = inputProps?.id ? inputProps.id : this._ariaMap.combobox;


nit:

Suggested change

const inputId = inputProps?.id ? inputProps.id : this._ariaMap.combobox;

const inputId = inputProps?.id ?? this._ariaMap.combobox;

khmakoto · 2024-11-21T22:07:25Z

packages/react/src/components/pickers/BasePicker.tsx

+  protected _getDescribedBy = (items: T[], hasError: boolean): string => {
+    const describedBy = [];
+    if (items.length > 0) {
+      describedBy.push(this._ariaMap.selectedItems);
+    }
+    if (hasError) {
+      describedBy.push(this._ariaMap.error);
+    }
+    return describedBy.join(' ');
+  };


nit: You could also achieve this, possibly even with better performance, by doing simple string concatenation instead of pushing into an array and joining.

khmakoto · 2024-11-21T22:08:54Z

packages/react/src/components/pickers/BasePicker.tsx

@@ -269,6 +282,7 @@ export class BasePicker<T extends {}, P extends IBasePickerProps<T>>

    const suggestionsVisible = !!this.state.suggestionsVisible;
    const suggestionsAvailable = suggestionsVisible ? this._ariaMap.suggestionList : undefined;
+    const hasError = typeof (this.state.errorMessage || this.props.errorMessage) !== 'undefined';


I think it's probably better if you define this as you did below:

Suggested change

const hasError = typeof (this.state.errorMessage || this.props.errorMessage) !== 'undefined';

const hasError = !!(this.state.errorMessage ?? this.props.errorMessage);

khmakoto · 2024-11-21T22:09:20Z

packages/react/src/components/pickers/BasePicker.tsx

+  }
+
+  protected renderError(className?: string): JSX.Element | null {
+    const errorMessage = this.props.errorMessage || this.state.errorMessage;


Should these be the other way around?

Suggested change

const errorMessage = this.props.errorMessage || this.state.errorMessage;

const errorMessage = this.state.errorMessage ?? this.props.errorMessage;

khmakoto · 2024-11-21T22:10:53Z

packages/react/src/components/pickers/BasePicker.tsx

    const { className, inputProps, disabled, selectionAriaLabel, selectionRole = 'list', theme, styles } = this.props;

    const suggestionsVisible = !!this.state.suggestionsVisible;

    const suggestionsAvailable: string | undefined = suggestionsVisible ? this._ariaMap.suggestionList : undefined;
+    const hasError = !!(this.state.errorMessage || this.props.errorMessage);


Suggested change

const hasError = !!(this.state.errorMessage || this.props.errorMessage);

const hasError = !!(this.state.errorMessage ?? this.props.errorMessage);

khmakoto · 2024-11-21T22:12:23Z

packages/react/src/components/pickers/BasePicker.tsx

+          error: css('ms-BasePicker-error', ''),
+          label: css('ms-BasePicker-label', ''),


Same thing as above, you don't need to use css here if you are only passing one classname

Suggested change

error: css('ms-BasePicker-error', ''),

label: css('ms-BasePicker-label', ''),

error: 'ms-BasePicker-error',

label: 'ms-BasePicker-label',

khmakoto · 2024-11-21T22:12:40Z

packages/react/src/components/pickers/BasePicker.tsx

@@ -1137,9 +1236,11 @@ export class BasePickerListBelow<T extends {}, P extends IBasePickerProps<T>> ex
        };

    const comboLabel = this.props['aria-label'] || inputProps?.['aria-label'];
+    const inputId = inputProps?.id ? inputProps.id : this._ariaMap.combobox;


nit:

Suggested change

const inputId = inputProps?.id ? inputProps.id : this._ariaMap.combobox;

const inputId = inputProps?.id ?? this._ariaMap.combobox;

tpalacino added 2 commits November 10, 2024 16:58

Updated pickers with input attribute compatibility.

2c670c2

Added change file.

6fc3f66

microsoft-github-policy-service bot added the Component: Pickers label Nov 10, 2024

tpalacino mentioned this pull request Nov 10, 2024

Issue 22528 picker required attribute #23362

Closed

tpalacino added 2 commits November 13, 2024 19:09

Merge branch 'master' into issue-22528-picker-input-attributes

535676b

Merge branch 'master' into issue-22528-picker-input-attributes

56ca291

tpalacino marked this pull request as ready for review November 16, 2024 13:49

tpalacino requested a review from a team as a code owner November 16, 2024 13:49

JustSlone assigned tudorpopams and unassigned tudorpopams Nov 16, 2024

smhigley requested changes Nov 19, 2024

View reviewed changes

microsoft-github-policy-service bot added the Needs: Author Feedback label Nov 19, 2024

Merge branch 'microsoft:master' into issue-22528-picker-input-attributes

77cd8e4

microsoft-github-policy-service bot removed the Needs: Author Feedback label Nov 19, 2024

khmakoto reviewed Nov 19, 2024

View reviewed changes

change/@fluentui-react-fd94592f-6c11-434f-a340-fce340db9a34.json Outdated Show resolved Hide resolved

tpalacino added 6 commits November 19, 2024 20:42

Resolved change description comment

75e270d

Removed ariaLabel property

f4e00a1

Updated label styles and removed id

9a16f7f

Removed default required error message

2962ad8

Enabled implementor to control error message display

f765a35

Connected error element to the aria-describedby

070bf48

Merge branch 'master' into issue-22528-picker-input-attributes

826c4f4

khmakoto reviewed Nov 21, 2024

View reviewed changes

tpalacino added 2 commits November 21, 2024 16:02

Merge branch 'master' into issue-22528-picker-input-attributes

745d388

Addressed PR feedback

79849b6

khmakoto reviewed Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated pickers with input attribute compatibility. #33243

Updated pickers with input attribute compatibility. #33243

tpalacino commented Nov 10, 2024

tpalacino commented Nov 16, 2024

smhigley left a comment

tpalacino commented Nov 19, 2024

khmakoto Nov 21, 2024

khmakoto Nov 21, 2024

khmakoto Nov 21, 2024

khmakoto Nov 21, 2024

khmakoto Nov 21, 2024

khmakoto Nov 21, 2024

khmakoto Nov 21, 2024

khmakoto Nov 21, 2024

		error: css('ms-BasePicker-error', ''),
		label: css('ms-BasePicker-label', ''),

	const inputId = inputProps?.id ? inputProps.id : this._ariaMap.combobox;
	const inputId = inputProps?.id ?? this._ariaMap.combobox;

	const hasError = typeof (this.state.errorMessage \|\| this.props.errorMessage) !== 'undefined';
	const hasError = !!(this.state.errorMessage ?? this.props.errorMessage);

	const errorMessage = this.props.errorMessage \|\| this.state.errorMessage;
	const errorMessage = this.state.errorMessage ?? this.props.errorMessage;

Updated pickers with input attribute compatibility. #33243

Are you sure you want to change the base?

Updated pickers with input attribute compatibility. #33243

Conversation

tpalacino commented Nov 10, 2024

Previous Behavior

New Behavior

Related Issue(s)

tpalacino commented Nov 16, 2024

smhigley left a comment

Choose a reason for hiding this comment

tpalacino commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment