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

RichSelectList: update color and make label optional #330

Merged
merged 12 commits into from Mar 15, 2024
4 changes: 4 additions & 0 deletions packages/syntax-core/src/RichSelect/RichSelect.module.css
Expand Up @@ -2,3 +2,7 @@
.richSelectBox:focus-visible {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jliotta could you make sure to add a changeset to this PR? pnpm run changeset

outline: none;
}

.label {
cursor: pointer;
}
11 changes: 10 additions & 1 deletion packages/syntax-core/src/RichSelect/RichSelectList.stories.tsx
Expand Up @@ -45,6 +45,7 @@ export const WhatIfItLookedLikeThis = (): ReactElement => {
<Box display="flex" gap={2} alignItems="center" flexWrap="wrap">
<RichSelectList
label="Label"
accessibilityLabel="Label"
helperText="Helper text"
multiple
placeholderText="Placeholder"
Expand Down Expand Up @@ -136,6 +137,7 @@ export const VeryLongContent = (): ReactElement => {
>
<RichSelectList
label="Label"
accessibilityLabel="Label"
helperText="Helper text"
placeholderText="Placeholder"
onChange={() => undefined}
Expand Down Expand Up @@ -205,7 +207,7 @@ export const Default: StoryObj<typeof RichSelectList> = {
args: {
children: renderOptions(),
helperText: "Helper text",
label: "Label",
accessibilityLabel: "Label",
placeholderText: "Placeholder",
selectedValues: ["opt1"],
},
Expand All @@ -226,6 +228,7 @@ const RichSelectListInteractive = (): ReactElement => {
return (
<RichSelectList
label="Label"
accessibilityLabel="Label"
helperText="Helper text"
multiple
selectedValues={selectionValue}
Expand Down Expand Up @@ -255,6 +258,7 @@ export const RadioButtons: StoryObj<typeof RichSelectList> = {
<RichSelectList
multiple={false}
label="Label"
accessibilityLabel="Label"
selectedValues={["opt1"]}
placeholderText="Placeholder"
onChange={() => undefined}
Expand All @@ -277,6 +281,7 @@ export const NoAutosaveControlled: StoryObj<typeof RichSelectList> = {
<RichSelectList
multiple
label="Label"
accessibilityLabel="Label"
selectedValues={["sf"]}
placeholderText="Placeholder"
helperText="When `autosave` is false, the user must click the button to save their changes"
Expand Down Expand Up @@ -337,6 +342,7 @@ export const NoAutosaveControlledMultipleSelect: StoryObj<
<ControlledRichSelectList
multiple
label="Multiple select"
accessibilityLabel="Label"
helperText="When `autosave` is false, the user must click the button to save their changes"
autosave={false}
>
Expand All @@ -348,6 +354,7 @@ export const NoAutosaveControlledMultipleSelect: StoryObj<
</ControlledRichSelectList>

<ControlledRichSelectList
accessibilityLabel="Label"
multiple={false}
label="Single select"
helperText="When `autosave` is false, the user must click the button to save their changes"
Expand All @@ -368,6 +375,7 @@ export const Autosave: StoryObj<typeof RichSelectList> = {
render: () => (
<RichSelectList
label="Label"
accessibilityLabel="Label"
helperText="When `autosave` is true, the user's changes are automatically saveted"
autosave
onChange={() => undefined}
Expand All @@ -389,6 +397,7 @@ export const ItemAttributeComposition: StoryObj<typeof RichSelectList> = {
render: () => (
<RichSelectList
label="Label"
accessibilityLabel="Label"
multiple
autosave
onChange={() => undefined}
Expand Down
Expand Up @@ -20,6 +20,7 @@ const user = userEvent.setup({
const defaultRequiredProps = {
onChange: () => undefined,
label: "x",
accessibilityLabel: "label",
primaryButtonText: "Save",
primaryButtonAccessibilityLabel: "Save",
secondaryButtonText: "Clear",
Expand Down
23 changes: 17 additions & 6 deletions packages/syntax-core/src/RichSelect/RichSelectList.tsx
Expand Up @@ -3,6 +3,7 @@ import React, {
useMemo,
type SyntheticEvent,
useRef,
useId,
} from "react";
import classNames from "classnames";
import {
Expand Down Expand Up @@ -59,7 +60,9 @@ export type RichSelectListProps = Omit<
/** Text shown below select box */
helperText?: string;
/** Text shown above select box */
label: string;
label?: string;
//** */
Copy link
Contributor

Choose a reason for hiding this comment

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

@jliotta instead of adding new accessibilityLabel prop, can we allow for an id to be passed in? This follows the same API as TextField:

The way folks can then add a label, is by adding a custom label outside of RichSelectList:

const reactId = useId();
const inputId = id ?? reactId;

accessibilityLabel: string;
/**
* Text showing in select box if no option has been chosen.
* There should always have a placeholder unless there is a default option selected
Expand Down Expand Up @@ -107,6 +110,7 @@ function RichSelectList(props: RichSelectListProps): ReactElement {
errorText,
helperText,
label,
accessibilityLabel,
onChange,
onClick = NOOP,
placeholderText,
Expand All @@ -117,6 +121,7 @@ function RichSelectList(props: RichSelectListProps): ReactElement {
...richSelectBoxProps
} = props;

const id = useId();
Copy link
Contributor

Choose a reason for hiding this comment

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

See

const reactId = useId();
const inputId = id ?? reactId;
- we want similar logic here

const isHydrated = useIsHydrated();
const disabled = !isHydrated || disabledProp;

Expand Down Expand Up @@ -186,14 +191,20 @@ function RichSelectList(props: RichSelectListProps): ReactElement {
setInteractionModality("keyboard"); // Show the focus ring so the user knows where focus went
}}
>
<Typography size={100} color="gray700">
{label}
</Typography>
{label && (
<label className={styles.label} htmlFor={id}>
<Box paddingX={1}>
<Typography size={100} color="gray700">
{label}
</Typography>
</Box>
</label>
)}
</ReactAriaLabel>
<Popover
ref={overlayHandlerRef}
disabled={disabled}
accessibilityLabel={label}
accessibilityLabel={accessibilityLabel}
content={
// this Box wrapper is to reapply the padding that was stripped from popover's dialog to show the sticky save/close buttons. Ideally this could be avoided
<Box
Expand All @@ -207,7 +218,7 @@ function RichSelectList(props: RichSelectListProps): ReactElement {
selectedValues={selectedKeys}
defaultSelectedValues={defaultSelectedKeys}
onChange={(selected) => setSelectedKeys(new Set(selected))}
accessibilityLabel={label}
accessibilityLabel={accessibilityLabel}
{...richSelectBoxProps}
>
{children}
Expand Down
1 change: 1 addition & 0 deletions packages/syntax-core/src/SelectList/SelectList.module.css
Expand Up @@ -32,6 +32,7 @@
outline: 0;
padding: 0 36px 0 12px;
width: 100%;
background-color: var(--color-base-white);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: can we alphabetically sort this?

}

.selectMouseFocusStyling {
Expand Down