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.

Sign up for GitHub

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

Jump to bottom

Allow inline fields with no visible label #2344

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+126 −19
Open

Allow inline fields with no visible label #2344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1a0d7a7
feat(odyssey-react-mui): allow TextField to not have visible label
bryancunningham-okta Apr 29, 2024
e9a0bd0
feat(odyssey-react-mui): allow Select to not have visible label
bryancunningham-okta Apr 29, 2024
83f3d0a
feat(odyssey-react-mui): allow hiding autocomplete label
bryancunningham-okta May 3, 2024
23bdd7b
feat(odyssey-react-mui): use prop from shared prop type
bryancunningham-okta Sep 6, 2024
4dcda7b
feat(odyssey-react-mui): dont show hint when no visible label
bryancunningham-okta Sep 9, 2024
ae45f1d
feat(odyssey-react-mui): make prop optional
bryancunningham-okta Sep 9, 2024
d6f094a
feat(odyssey-react-mui): allow no label on native select
bryancunningham-okta Sep 9, 2024
220670e
feat(odyssey-react-mui): update prop description
bryancunningham-okta Sep 9, 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
5 changes: 4 additions & 1 deletion packages/odyssey-react-mui/src/Autocomplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ export type AutocompleteProps<
FieldComponentProps,
| "errorMessage"
| "errorMessageList"
| "hasVisibleLabel"
| "hint"
| "HintLinkComponent"
| "id"
Expand All @@ -218,6 +219,7 @@ const Autocomplete = <
errorMessage,
errorMessageList,
hasMultipleChoices,
hasVisibleLabel,
id: idOverride,
inputValue,
isCustomValueAllowed,
Expand Down Expand Up @@ -299,7 +301,7 @@ const Autocomplete = <
errorMessage={errorMessage}
errorMessageList={errorMessageList}
fieldType="single"
hasVisibleLabel
hasVisibleLabel={hasVisibleLabel}
//@ts-expect-error htmlFor does not exist ont he InputLabelProps for autocomplete
id={InputLabelProps.htmlFor}
isFullWidth={isFullWidth}
Expand Down Expand Up @@ -334,6 +336,7 @@ const Autocomplete = <
ariaDescribedBy,
errorMessage,
errorMessageList,
hasVisibleLabel,
hint,
HintLinkComponent,
isFullWidth,
Expand Down
11 changes: 4 additions & 7 deletions packages/odyssey-react-mui/src/Field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@ export type FieldProps = {
* The field type determines how ARIA components are setup. It's important to use this to denote if you expect only one component (like a text field) or multiple (like a radio group).
*/
fieldType: (typeof fieldTypeValues)[number];
/**
* If `true`, the Field label will be shown
*/
hasVisibleLabel: boolean;
/**
* Important for narrowing down the `fieldset` role to "radiogroup".
*/
Expand Down Expand Up @@ -76,14 +72,14 @@ export type FieldProps = {
labelElementId,
isReadOnly,
}: RenderFieldComponentProps) => ReactElement;
};
} & Pick<FieldComponentProps, "hasVisibleLabel">;

const Field = ({
ariaDescribedBy,
errorMessage,
errorMessageList,
fieldType,
hasVisibleLabel,
hasVisibleLabel = true,
hint,
HintLinkComponent,
id: idOverride,
Expand All @@ -99,6 +95,7 @@ const Field = ({
FieldComponentProps,
| "errorMessage"
| "errorMessageList"
| "hasVisibleLabel"
| "hint"
| "HintLinkComponent"
| "id"
Expand Down Expand Up @@ -160,7 +157,7 @@ const Field = ({
/>
)}

{hint && (
{hint && hasVisibleLabel && (
<FieldHint id={hintId} LinkComponent={HintLinkComponent} text={hint} />
)}

Expand Down
47 changes: 40 additions & 7 deletions packages/odyssey-react-mui/src/FieldComponentProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,11 @@ export type FieldComponentProps = {
*/
errorMessageList?: string[];
/**
* The helper text content.
* If `false` no visible label is shown.
*
* NOTE: the `label` prop is still required for accessibility purposes
*/
hint?: string;
/**
* A `Link` component to provide greater context that is rendered at the end of the `hint` text
*/
HintLinkComponent?: ReactElement<typeof HintLink>;
hasVisibleLabel?: boolean;
/**
* The id of the `input` element.
*/
Expand All @@ -55,7 +53,30 @@ export type FieldComponentProps = {
* The name of the `input` element. Defaults to the `id` if not set.
*/
name?: string;
};
} & (
| {
/**
* If `hasVisibleLabel` is `false` it will also hide any `hint` text and `HintLinkComponent` that is passed in
*/
hasVisibleLabel?: false;
hint: never;
HintLinkComponent: never;
}
| {
/**
* defaults to `true` if `false` along with hiding the label it will also hide any `hint` text and `HintLinkComponent` that is passed in
*/
hasVisibleLabel?: true;
/**
* The helper text content.
*/
hint?: string;
/**
* A `Link` component to provide greater context that is rendered at the end of the `hint` text
*/
HintLinkComponent?: ReactElement<typeof HintLink>;
}
);
Comment on lines +56 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible we could rewrite this type to use the function overload syntax? Or have 2 different Field components, one with a label and one that only takes aria-labelledby or something like that?

The way the code works today, it's very confusing.

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 would be a pretty significant rewrite. We'd have to refactor all the form field components(that allow a non-visible label) to handle this 'correctly'


export type FieldComponentRenderProps = {
ariaDescribedBy: string;
Expand All @@ -64,3 +85,15 @@ export type FieldComponentRenderProps = {
id: string;
labelElementId: string;
};

export type ConditionalLabelProps =
| {
hasVisibleLabel: false;
hint?: never;
placeholder: string;
}
Comment on lines +90 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we expecting they at least pass in aria-labelledby or aria-label if they have no visible label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the actual label prop is still required. It's just visually hidden

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, my bad. I see. So we're just saying if the label is visible, not if it doesn't exist. My bad.

We should also require aria-labelledby if no label is passed in as an alternative. We can keep visually hiding the label, but it'd be good to say our intent is that they have to have a visual label somewhere else. That's what I mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aria-labelledby will be controlled by us. It'll still be labelled by the non-visible label

| {
hasVisibleLabel?: true;
hint?: string;
placeholder?: string;
};
4 changes: 3 additions & 1 deletion packages/odyssey-react-mui/src/NativeSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export type NativeSelectProps<
FieldComponentProps,
| "errorMessage"
| "errorMessageList"
| "hasVisibleLabel"
| "hint"
| "HintLinkComponent"
| "id"
Expand All @@ -123,6 +124,7 @@ const NativeSelect = forwardRef(
errorMessage,
errorMessageList,
hasMultipleChoices: hasMultipleChoicesProp,
hasVisibleLabel = true,
hint,
HintLinkComponent,
id: idOverride,
Expand Down Expand Up @@ -233,7 +235,7 @@ const NativeSelect = forwardRef(
errorMessage={errorMessage}
errorMessageList={errorMessageList}
fieldType="single"
hasVisibleLabel
hasVisibleLabel={hasVisibleLabel}
hint={hint}
HintLinkComponent={HintLinkComponent}
id={idOverride}
Expand Down
4 changes: 3 additions & 1 deletion packages/odyssey-react-mui/src/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ export type SelectProps<
FieldComponentProps,
| "errorMessage"
| "errorMessageList"
| "hasVisibleLabel"
| "hint"
| "HintLinkComponent"
| "id"
Expand Down Expand Up @@ -204,6 +205,7 @@ const Select = <
errorMessage,
errorMessageList,
hasMultipleChoices: hasMultipleChoicesProp,
hasVisibleLabel,
hint,
HintLinkComponent,
id: idOverride,
Expand Down Expand Up @@ -515,7 +517,7 @@ const Select = <
errorMessage={errorMessage}
errorMessageList={errorMessageList}
fieldType="single"
hasVisibleLabel
hasVisibleLabel={hasVisibleLabel}
hint={hint}
HintLinkComponent={HintLinkComponent}
id={idOverride}
Expand Down
7 changes: 5 additions & 2 deletions packages/odyssey-react-mui/src/TextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
import { InputAdornment, InputBase } from "@mui/material";

import {
ConditionalLabelProps,
FieldComponentProps,
FieldComponentRenderProps,
} from "./FieldComponentProps";
Expand Down Expand Up @@ -154,7 +155,8 @@ export type TextFieldProps = {
*/
value?: string;
} & FieldComponentProps &
Pick<HtmlProps, "ariaDescribedBy" | "testId" | "translate">;
Pick<HtmlProps, "ariaDescribedBy" | "testId" | "translate"> &
ConditionalLabelProps;

type FieldRenderProps = Partial<
Pick<FieldComponentRenderProps, "ariaDescribedBy" | "errorMessageElementId">
Expand All @@ -171,6 +173,7 @@ const TextField = forwardRef<HTMLInputElement, TextFieldProps>(
endAdornment,
errorMessage,
errorMessageList,
hasVisibleLabel = true,
hint,
HintLinkComponent,
id: idOverride,
Expand Down Expand Up @@ -305,7 +308,7 @@ const TextField = forwardRef<HTMLInputElement, TextFieldProps>(
errorMessage={errorMessage}
errorMessageList={errorMessageList}
fieldType="single"
hasVisibleLabel
hasVisibleLabel={hasVisibleLabel}
hint={hint}
HintLinkComponent={HintLinkComponent}
id={idOverride}
Expand Down
15 changes: 15 additions & 0 deletions packages/odyssey-storybook/src/components/odyssey-mui/Autocomplete/Autocomplete.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const storybookMeta: Meta<typeof Autocomplete> = {
},
},
},
hasVisibleLabel: fieldComponentPropsMetaData.hasVisibleLabel,
hint: fieldComponentPropsMetaData.hint,
HintLinkComponent: fieldComponentPropsMetaData.HintLinkComponent,
id: fieldComponentPropsMetaData.id,
Expand Down Expand Up @@ -448,6 +449,20 @@ const jupiterGalileanMoons: MoonMeta[] = [
},
];

export const NoVisibleLabel: StoryObj<JupiterMoonsAutocomplete> = {
parameters: {
docs: {
description: {
story:
"TextFields with `hasVisibleLabel` set to false will not render a visible label. Although, the `label` prop is still required for accessibility purposes",
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
"TextFields with `hasVisibleLabel` set to false will not render a visible label. Although, the `label` prop is still required for accessibility purposes",
"`TextField`s with `hasVisibleLabel` set to false will not render a visible label. Although, the `label` prop is still required for accessibility purposes.",

Copy link
Contributor

Choose a reason for hiding this comment

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

There are other spots in the docs like this as well.

},
},
},
args: {
hasVisibleLabel: false,
},
};

export const ControlledMultipleAutocomplete: StoryObj<JupiterMoonsAutocomplete> =
{
parameters: {
Expand Down
8 changes: 8 additions & 0 deletions packages/odyssey-storybook/src/components/odyssey-mui/NativeSelect/NativeSelect.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const storybookMeta: Meta<typeof NativeSelect> = {
},
errorMessage: fieldComponentPropsMetaData.errorMessage,
errorMessageList: fieldComponentPropsMetaData.errorMessageList,
hasVisibleLabel: fieldComponentPropsMetaData.hasVisibleLabel,
hint: fieldComponentPropsMetaData.hint,
HintLinkComponent: fieldComponentPropsMetaData.HintLinkComponent,
id: fieldComponentPropsMetaData.id,
Expand Down Expand Up @@ -203,6 +204,13 @@ export const DefaultDisabled: StoryObj<typeof NativeSelect> = {
},
};

export const NoVisibleLabel: StoryObj<typeof NativeSelect> = {
...Template,
args: {
hasVisibleLabel: false,
},
};

export const DefaultError: StoryObj<typeof NativeSelect> = {
...Template,
args: {
Expand Down
15 changes: 15 additions & 0 deletions packages/odyssey-storybook/src/components/odyssey-mui/Select/Select.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ const storybookMeta: Meta<SelectProps<string | string[], boolean>> = {
},
},
},
hasVisibleLabel: fieldComponentPropsMetaData.hasVisibleLabel,
hint: fieldComponentPropsMetaData.hint,
HintLinkComponent: fieldComponentPropsMetaData.HintLinkComponent,
id: fieldComponentPropsMetaData.id,
Expand Down Expand Up @@ -476,3 +477,17 @@ export const ControlledPreselectedMultipleSelect: StoryObj<typeof Select> = {
return <Select {...props} value={localValue} onChange={onChange} />;
},
};

export const NoVisibleLabel: StoryObj<typeof Select> = {
parameters: {
docs: {
description: {
story:
"Selects with `hasVisibleLabel` set to false will not render a visible label. Although, the `label` prop is still required for accessibility purposes",
},
},
},
args: {
hasVisibleLabel: false,
},
};
17 changes: 17 additions & 0 deletions packages/odyssey-storybook/src/components/odyssey-mui/TextField/TextField.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const storybookMeta: Meta<typeof TextField> = {
},
},
},
hasVisibleLabel: fieldComponentPropsMetaData.hasVisibleLabel,
hint: fieldComponentPropsMetaData.hint,
HintLinkComponent: fieldComponentPropsMetaData.HintLinkComponent,
id: fieldComponentPropsMetaData.id,
Expand Down Expand Up @@ -177,6 +178,7 @@ const storybookMeta: Meta<typeof TextField> = {
},
args: {
label: "Destination",
hint: "Specify your destination within the Sol system.",
defaultValue: undefined,
},
decorators: [MuiThemeDecorator],
Expand Down Expand Up @@ -299,6 +301,7 @@ export const Email: StoryObj<typeof TextField> = {
label: "Company email",
type: "email",
defaultValue: "",
hint: undefined,
},
};

Expand Down Expand Up @@ -326,6 +329,20 @@ export const Placeholder: StoryObj<typeof TextField> = {
},
};

export const NoVisibleLabel: StoryObj<typeof TextField> = {
parameters: {
docs: {
description: {
story:
"TextFields with `hasVisibleLabel` set to false will not render a visible label. Although, the `label` prop is still required for accessibility purposes",
},
},
},
args: {
hasVisibleLabel: false,
},
};

export const Tel: StoryObj<typeof TextField> = {
parameters: {
docs: {
Expand Down
12 changes: 12 additions & 0 deletions packages/odyssey-storybook/src/fieldComponentPropsMetaData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@ export const fieldComponentPropsMetaData: Partial<
},
},
},
hasVisibleLabel: {
control: "boolean",
description: "If `false` no visible label is shown.",
table: {
type: {
summary: "boolean",
},
defaultValue: {
summary: true,
},
},
},
hint: {
control: "text",
description: "The helper text content",
Expand Down