Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read-only support for Checkbox, Radio, Switch [OKTA-731946] #2305

Merged
merged 22 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
30e97ef
fix: initial logic
benlister-okta Jul 16, 2024
c306ca3
fix: add styles and update logic for checkbox
benlister-okta Jul 18, 2024
d8d70d9
fix: add readOnly to checkbox and radio groups
benlister-okta Jul 20, 2024
7e2da86
fix: add readOnly to switch
benlister-okta Jul 22, 2024
30ee68b
fix: remove unneeded comments
benlister-okta Jul 26, 2024
d68131f
fix: style updates, comments
benlister-okta Jul 26, 2024
c30533c
fix: tabindex, comments
benlister-okta Jul 26, 2024
c38b805
fix: updates from pr review
benlister-okta Jul 29, 2024
9e7a575
fix: swap on mousedown for onclick
benlister-okta Jul 30, 2024
dacf3f7
fix: update switch
benlister-okta Jul 30, 2024
2cf8f3c
fix: switch updates from pr feedback
benlister-okta Jul 30, 2024
5fc601c
fix: replace read-only with disabled aria in Radio
benlister-okta Jul 31, 2024
f66a53a
fix: add comments
benlister-okta Aug 1, 2024
7014df6
fix: updates based on PR feedback
benlister-okta Aug 5, 2024
f8e01fe
fix: adding pr review feedback
benlister-okta Aug 5, 2024
5691113
fix: add read-only support to Select
benlister-okta Aug 9, 2024
9e5c9ad
fix: pass readonly into select chips object
benlister-okta Aug 9, 2024
9c2b32b
Merge remote-tracking branch 'origin/main' into bl_read_only_forms_OK…
benlister-okta Aug 9, 2024
37efc4f
fix: simplify logic, remove redundancies
benlister-okta Aug 16, 2024
2485d7f
feat(odyssey-react-mui): remove internal state from RadioGroup
bryancunningham-okta Aug 21, 2024
8a99902
fix: remove commented playwright test
benlister-okta Aug 21, 2024
760edcf
fix: update integration tests to match content
benlister-okta Aug 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 39 additions & 9 deletions packages/odyssey-react-mui/src/Checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ export type CheckboxProps = {
* Callback fired when the blur event happens. Provides event value.
*/
onBlur?: MuiFormControlLabelProps["onBlur"];
} & Pick<FieldComponentProps, "hint" | "id" | "isDisabled" | "name"> &
} & Pick<
FieldComponentProps,
"hint" | "id" | "isDisabled" | "isReadOnly" | "name"
> &
CheckedFieldProps<MuiCheckboxProps> &
Pick<HtmlProps, "ariaLabel" | "ariaLabelledBy" | "testId" | "translate">;

Expand All @@ -74,6 +77,7 @@ const Checkbox = ({
isDefaultChecked,
isDisabled,
isIndeterminate,
isReadOnly = false,
isRequired,
label: labelProp,
hint,
Expand Down Expand Up @@ -102,13 +106,11 @@ const Checkbox = ({
const localInputRef = useRef<HTMLInputElement>(null);
useImperativeHandle(
inputRef,
() => {
return {
focus: () => {
localInputRef.current?.focus();
},
};
},
() => ({
focus: () => {
localInputRef.current?.focus();
},
}),
[],
);

Expand Down Expand Up @@ -136,16 +138,39 @@ const Checkbox = ({
[onChangeProp],
);

const onClick = useCallback<NonNullable<MuiCheckboxProps["onClick"]>>(
(event) => {
if (isReadOnly) {
event.stopPropagation();
event.preventDefault();
}
},
[isReadOnly],
);

const onBlur = useCallback<NonNullable<MuiFormControlLabelProps["onBlur"]>>(
(event) => {
onBlurProp?.(event);
},
[onBlurProp],
);

const checkboxStyles = useMemo(
() => ({
alignItems: "flex-start",
...(isReadOnly && {
cursor: "default",
"& .MuiTypography-root": {
cursor: "default",
},
}),
}),
[isReadOnly],
);

return (
<FormControlLabel
sx={{ alignItems: "flex-start" }}
sx={checkboxStyles}
aria-label={ariaLabel}
aria-labelledby={ariaLabelledBy}
className={
Expand All @@ -160,9 +185,14 @@ const Checkbox = ({
{...inputValues}
indeterminate={isIndeterminate}
onChange={onChange}
onClick={
onClick as unknown as React.MouseEventHandler<HTMLButtonElement>
}
Comment on lines +188 to +190
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta Aug 5, 2024

Choose a reason for hiding this comment

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

Weird. Any way around this syntax like setting our useCallback to MuiCheckboxProps["onClick"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. I think this was part of something I was debugging that didn't get removed. I updated the onClick to match the existing onChange:

const onClick = useCallback<NonNullable<MuiCheckboxProps["onClick"]>>(
    (event) => {
      if (isReadOnly) {
        event.preventDefault();
      }
    },
    [isReadOnly],
  );

required={isRequired}
inputProps={{
"data-se": testId,
"aria-readonly": isReadOnly,
readOnly: isReadOnly,
}}
Comment on lines 192 to 196
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we memoize these inputProps? I can't see the whole file, so it might not make sense, but it looks like it could be memoized. If it's in Field's function, then it's already memoized. My bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could memoize them, but since aria-readonly is being used on the native element via inputProps, and isReadOnly is already in memory, since it's used elsewhere, I wonder if there is much of a benefit of it. What do you think @KevinGhadyani-Okta?

disabled={isDisabled}
inputRef={localInputRef}
Expand Down
24 changes: 20 additions & 4 deletions packages/odyssey-react-mui/src/CheckboxGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
* See the License for the specific language governing permissions and limitations under the License.
*/

import React, { memo, ReactNode, useCallback, useMemo } from "react";
import { FormGroup as MuiFormGroup } from "@mui/material";
import { memo, ReactNode, useCallback } from "react";

import { CheckboxProps } from "./Checkbox";
import { Field } from "./Field";
import {
FieldComponentProps,
Expand Down Expand Up @@ -41,6 +41,7 @@ export type CheckboxGroupProps = {
| "HintLinkComponent"
| "id"
| "isDisabled"
| "isReadOnly"
> &
Pick<HtmlProps, "ariaDescribedBy" | "testId" | "translate">;

Expand All @@ -61,11 +62,26 @@ const CheckboxGroup = ({
HintLinkComponent,
id: idOverride,
isDisabled,
isReadOnly = false,
isRequired = false,
label,
testId,
translate,
}: CheckboxGroupProps) => {
const memoizedChildren = useMemo(
() =>
React.Children.map(children, (child) => {
if (React.isValidElement<CheckboxProps>(child)) {
return React.cloneElement(child, {
isReadOnly,
isDisabled: isDisabled || child.props.isDisabled,
});
}
return child;
}),
[children, isReadOnly, isDisabled],
);

const renderFieldComponent = useCallback(
({
ariaDescribedBy,
Expand All @@ -81,10 +97,10 @@ const CheckboxGroup = ({
id={id}
translate={translate}
>
{children}
{memoizedChildren}
</MuiFormGroup>
),
[children, testId, translate],
[memoizedChildren, testId, translate],
);

return (
Expand Down
7 changes: 5 additions & 2 deletions packages/odyssey-react-mui/src/Field.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,10 @@
*/

import { memo, ReactElement, useMemo } from "react";

import {
FormControl as MuiFormControl,
FormLabel as MuiFormLabel,
} from "@mui/material";

import { FieldComponentProps } from "./FieldComponentProps";
import { FieldError } from "./FieldError";
import { FieldHint } from "./FieldHint";
Expand All @@ -35,6 +33,7 @@ export type RenderFieldComponentProps = {
errorMessageElementId?: string;
id: string;
labelElementId: string;
isReadOnly?: boolean;
};

export type FieldProps = {
Expand Down Expand Up @@ -75,6 +74,7 @@ export type FieldProps = {
errorMessageElementId,
id,
labelElementId,
isReadOnly,
}: RenderFieldComponentProps) => ReactElement;
};

Expand All @@ -91,6 +91,7 @@ const Field = ({
isFullWidth = false,
isRadioGroup = false,
isOptional = false,
isReadOnly = false,
label,
renderFieldComponent,
}: FieldProps &
Expand All @@ -104,6 +105,7 @@ const Field = ({
| "isDisabled"
| "isFullWidth"
| "isOptional"
| "isReadOnly"
> &
Pick<HtmlProps, "ariaDescribedBy">) => {
const { t } = useTranslation();
Expand Down Expand Up @@ -167,6 +169,7 @@ const Field = ({
errorMessageElementId,
id,
labelElementId,
isReadOnly,
})}

{(errorMessage || errorMessageList) && (
Expand Down
55 changes: 42 additions & 13 deletions packages/odyssey-react-mui/src/Radio.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ import {
Radio as MuiRadio,
RadioProps as MuiRadioProps,
} from "@mui/material";

import { memo, useCallback, useMemo, useRef, useImperativeHandle } from "react";

import { FieldComponentProps } from "./FieldComponentProps";
import type { HtmlProps } from "./HtmlProps";
import { FocusHandle } from "./inputUtils";
Expand All @@ -31,11 +29,11 @@ export type RadioProps = {
*/
inputRef?: React.RefObject<FocusHandle>;
/**
* If `true`, the Radio is selected
* Determines whether the Radio button is checked
*/
isChecked?: boolean;
/**
* If `true`, the Radio has an invalid value
* If `true`, the radio button has an invalid value
*/
isInvalid?: boolean;
/**
Expand All @@ -46,30 +44,33 @@ export type RadioProps = {
* The value attribute of the Radio
*/
value: string;
/**
* Callback fired when the state is changed. Provides event and checked value.
*/
onChange?: MuiRadioProps["onChange"];
/**
* Callback fired when the blur event happens. Provides event value.
*/
onChange?: MuiRadioProps["onChange"];
onBlur?: MuiFormControlLabelProps["onBlur"];
} & Pick<FieldComponentProps, "hint" | "id" | "isDisabled" | "name"> &
onClick?: React.MouseEventHandler<HTMLSpanElement>;
} & Pick<
FieldComponentProps,
"hint" | "id" | "isDisabled" | "isReadOnly" | "name"
> &
Pick<HtmlProps, "testId" | "translate">;

const Radio = ({
hint,
inputRef,
isChecked,
isDisabled,
isDisabled = false,
isInvalid,
label: labelProp,
name,
testId,
translate,
value,
isReadOnly = false,
onChange: onChangeProp,
onBlur: onBlurProp,
onClick,
}: RadioProps) => {
const localInputRef = useRef<HTMLInputElement>(null);
useImperativeHandle(
Expand All @@ -95,10 +96,25 @@ const Radio = ({
);

const onChange = useCallback<NonNullable<MuiRadioProps["onChange"]>>(
(event, checked) => onChangeProp?.(event, checked),
[onChangeProp],
(event, checked) => {
if (isReadOnly) {
event.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can add event.stopPropagation(); here if we want. May not be necessary but also won't hurt anything

} else {
onChangeProp?.(event, checked);
}
},
[onChangeProp, isReadOnly],
);

const handleClick = useCallback<React.MouseEventHandler<HTMLSpanElement>>(
(event) => {
if (isReadOnly) {
event.stopPropagation();
event.preventDefault();
}
},
[isReadOnly],
);
const onBlur = useCallback<NonNullable<MuiFormControlLabelProps["onBlur"]>>(
(event) => {
onBlurProp?.(event);
Expand All @@ -114,17 +130,30 @@ const Radio = ({
<MuiRadio
inputProps={{
"data-se": testId,
"aria-disabled": isDisabled || isReadOnly,
readOnly: isReadOnly,
tabIndex: isReadOnly ? 0 : undefined,
}}
inputRef={localInputRef}
onChange={onChange}
onClick={onClick || handleClick}
disabled={isDisabled}
/>
}
disabled={isDisabled}
label={label}
name={name}
translate={translate}
value={value}
onBlur={onBlur}
disabled={isDisabled}
sx={{
...(isReadOnly && {
cursor: "default",
"& .MuiTypography-root": {
cursor: "default",
},
}),
}}
Comment on lines +149 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, these styles are in components.tsx so they apply to all inputs. Can we move them in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This style only applies to the label, not the form elements. I wanted this to live in components.tsx but found I was not able to target from the MuiCheckbox/MuiRadio style blocks, since its applied outside of the component. I believe we'd have to re-factor how the MuiFormControlLabel works to accept the isReadOnly prop so I figure this is the easier and cleaner path since it only applies to checkboxes and radios.

/>
);
};
Expand Down
Loading