fix: move testId to inputProps for all input components #2096

trevoring-okta · 2024-01-22T19:48:33Z

Summary

Existing implementation of testId had placed it on the wrapper MUI element for all MUI components that wrap an underlying raw HTML input element (e.g. TextField, Checkbox, Radio, etc.). Most downstream repos have written their unit and e2e tests expecting that the data-se attribute is on the raw HTML input element rather than a wrapper parent element, so this PR moves data-se to MUI's inputProps
- Required module augmentation of React's HTMLAttributes to allow data-* attributes because of an underlying issue with TypeScript
- Moved FocusHandle out of declaration file because it fits better in inputUtils.ts
- Fixes missing input focus ref and autocomplete props from NativeSelect

bryancunningham-okta

This seems fine to me. Asking for @chrispulsinelli-okta for some help getting an approval for this

trevoring-okta · 2024-01-25T21:36:23Z

packages/odyssey-react-mui/src/labs/VirtualizedAutocomplete.tsx

-    [errorMessage, hint, isOptional, label, nameOverride]
+    [
+      errorMessage,
+      errorMessageList,


lint fix here @bryancunningham-okta @chrispulsinelli-okta

chrispulsinelli-okta

I'm OK with this change and leaving an approval as to not block you, but please review and update, if necessary, the change to the import statement in react-augment.d.ts please. Once that is addressed, this should be fine to merge.

chrispulsinelli-okta · 2024-01-25T21:36:06Z

packages/odyssey-react-mui/src/@types/react-augment.d.ts

@@ -10,14 +10,18 @@
 * See the License for the specific language governing permissions and limitations under the License.
 */

-import { FC } from "react";
-
+import "react";


Any reason why you removed FC as an imported object? I believe this affects the interface definition below where it extends FC

I believe this should be something like import React, { FC } from "react";

@chrispulsinelli-okta So this change to react-augment.d.ts came from a discussion with Kevin. I saw that you were the original author of this file but Kevin was saying he thought the ForwardRefWithType definition would actually fit better in inputUtils.ts (just like what I did with FocusHandle) since d.ts files shouldn't be used for holding custom types. I'm not a TS expert so I'm most relaying what Kevin was saying, but he originally wanted me to move your ForwardRefWithType interface into inputUtils.ts in this ticket as well but it led to a bunch of TS compiler errors when I tried to do so since Odyssey has "skipLibCheck": true in tsconfig which prevents type checking in d.ts files.

I don't have any context behind ForwardRefWithType and didn't want to break your code so Kevin asked me to create a ticket and assign it to you for this sprint: https://oktainc.atlassian.net/browse/OKTA-688308

Regardless, I can still add back the FC for the scope of this PR

chrispulsinelli-okta · 2024-01-25T21:42:42Z

You have the following errors in the build:

Error: src/labs/DatePicker.tsx(21,10): error TS2590: Expression produces a union type that is too complex to represent.
lerna ERR! yarn run typecheck exited 2 in '@okta/odyssey-react-mui'

trevoring-okta · 2024-01-25T22:02:17Z

You have the following errors in the build:

Error: src/labs/DatePicker.tsx(21,10): error TS2590: Expression produces a union type that is too complex to represent.
lerna ERR! yarn run typecheck exited 2 in '@okta/odyssey-react-mui'

@chrispulsinelli-okta I didn't touch the DatePicker component as part of this PR but I can still try to resolve the issue. Do you anticipate this blocking the publishing of this PR if it doesn't get resolved?

UPDATE: This build error actually did result from changes to react-augment.d.ts, they have been fixed

trevoring-okta · 2024-01-29T17:19:42Z

packages/odyssey-react-mui/src/NativeSelect.tsx

          id={idOverride}
          inputProps={{
            "aria-errormessage": errorMessageElementId,
            "aria-labelledby": labelElementId,
+            "data-se": testId,


@KevinGhadyani-Okta

trevoring-okta · 2024-01-29T17:19:55Z

packages/odyssey-react-mui/src/Select.tsx

        id={id}
+        inputProps={{ "data-se": testId }}


@KevinGhadyani-Okta

KevinGhadyani-Okta · 2024-01-29T21:54:03Z

packages/odyssey-react-mui/src/@types/react-augment.d.ts

+type DataAttributeKey = `data-${string}`;
+declare module "react" {
+  interface InputHTMLAttributes<T> extends HTMLAttributes<T> {


I will approve, but please check if the type definition at line 20 is supposed to go inside the declare at line 21.

I didn't notice it before, but I'm thinking that's how you localize global type overrides.

KevinGhadyani-Okta · 2024-01-29T21:54:39Z

packages/odyssey-react-mui/src/NativeSelect.tsx

+  /**
+   * This prop helps users to fill forms faster, especially on mobile devices.
+   * The name can be confusing, as it's more like an autofill.
+   * @see https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofill


Nice documentation comment! 👍

KevinGhadyani-Okta · 2024-01-29T21:55:37Z

packages/odyssey-react-mui/src/NativeSelect.tsx

      defaultValue,
      errorMessage,
      errorMessageList,
      hasMultipleChoices: hasMultipleChoicesProp,
      hint,
      HintLinkComponent,
      id: idOverride,
+      inputFocusRef,


I'd like this to be more generic.

Can we rename this inputRef so maybe .blur() is possible in the future and others?

The internal one could be named localInputRef to differentiate it.

KevinGhadyani-Okta · 2024-01-29T21:55:58Z

packages/odyssey-react-mui/src/NativeSelect.tsx

+        const element = inputRef.current;
+        return {
+          focus: () => {
+            element && element.focus();


I'd like to have this match other places in the code:

Suggested change

element && element.focus();

element?.focus();

KevinGhadyani-Okta · 2024-01-29T21:56:18Z

packages/odyssey-react-mui/src/NativeSelect.tsx

+    useImperativeHandle(
+      inputFocusRef,
+      () => {
+        const element = inputRef.current;


I don't think we need to create this element variable.

I'm fine having inputRef.current?.focus() below.

KevinGhadyani-Okta

A few comments, but it's otherwise very close! :)

trevoring-okta requested review from KevinGhadyani-Okta, benschell-okta, chrispulsinelli-okta and a team as code owners January 22, 2024 19:48

bryancunningham-okta previously approved these changes Jan 25, 2024

View reviewed changes

trevoring-okta commented Jan 25, 2024

View reviewed changes

chrispulsinelli-okta previously approved these changes Jan 25, 2024

View reviewed changes

trevoring-okta dismissed stale reviews from chrispulsinelli-okta and bryancunningham-okta via 7c50d3d January 26, 2024 05:13

trevoring-okta added 3 commits January 29, 2024 11:54

fix: move testId to inputProps for all input components

04c5a8b

fix: build issue

eaba3bf

fix: add support for autocomplete and inputref in NativeSelect

f954b6c

trevoring-okta force-pushed the ti-move-input-components-testid branch from d8a3bdc to f954b6c Compare January 29, 2024 16:56

fix: lint fix

cb1f482

trevoring-okta commented Jan 29, 2024

View reviewed changes

packages/odyssey-react-mui/src/Select.tsx

id={id}

inputProps={{ "data-se": testId }}

Copy link

Contributor Author

trevoring-okta Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KevinGhadyani-Okta

KevinGhadyani-Okta reviewed Jan 29, 2024

View reviewed changes

fix: address PR comments

b3ec07a

KevinGhadyani-Okta approved these changes Jan 29, 2024

View reviewed changes

trevoring-okta merged commit 94ec2ea into main Jan 29, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: move testId to inputProps for all input components #2096

fix: move testId to inputProps for all input components #2096

trevoring-okta commented Jan 22, 2024 •

edited

Loading

bryancunningham-okta left a comment

trevoring-okta Jan 25, 2024

chrispulsinelli-okta left a comment

chrispulsinelli-okta Jan 25, 2024

chrispulsinelli-okta Jan 25, 2024

trevoring-okta Jan 25, 2024

chrispulsinelli-okta commented Jan 25, 2024

trevoring-okta commented Jan 25, 2024 •

edited

Loading

trevoring-okta Jan 29, 2024

trevoring-okta Jan 29, 2024

KevinGhadyani-Okta Jan 29, 2024

KevinGhadyani-Okta Jan 29, 2024

KevinGhadyani-Okta Jan 29, 2024

KevinGhadyani-Okta Jan 29, 2024

KevinGhadyani-Okta Jan 29, 2024

KevinGhadyani-Okta left a comment

fix: move testId to inputProps for all input components #2096

fix: move testId to inputProps for all input components #2096

Conversation

trevoring-okta commented Jan 22, 2024 • edited Loading

Summary

bryancunningham-okta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispulsinelli-okta left a 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

chrispulsinelli-okta commented Jan 25, 2024

trevoring-okta commented Jan 25, 2024 • edited Loading

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

KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

trevoring-okta commented Jan 22, 2024 •

edited

Loading

trevoring-okta commented Jan 25, 2024 •

edited

Loading