Skip to content

Commit

Permalink
fix(option): makes value and text props optional
Browse files Browse the repository at this point in the history
Updates Option interface to allow `value` and `text` to be optional to allow users
to compose `children` for complex layouts. This ensures that `SelectList` height
is set relatively when a custom option is passed as child so that there is no
overflow

fix #6939
  • Loading branch information
edleeks87 committed Oct 14, 2024
1 parent c51688f commit fb37215
Show file tree
Hide file tree
Showing 9 changed files with 173 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -291,11 +291,12 @@ const SelectList = React.forwardRef(

const optionChildrenList = useMemo(
() =>
childrenList.filter(
(child) =>
childrenList.filter((child) => {
return (
React.isValidElement(child) &&
(child.type === Option || child.type === OptionRow)
),
);
}),
[childrenList]
);

Expand Down
4 changes: 3 additions & 1 deletion src/components/select/__internal__/utils/with-filter.hoc.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,9 @@ const withFilter = <T extends WrappedComponentProps>(
}

return (
<StyledOption>{noResultsMessage || noResultsText}</StyledOption>
<StyledOption isInteractive>
{noResultsMessage || noResultsText}
</StyledOption>
);
}

Expand Down
13 changes: 7 additions & 6 deletions src/components/select/option/option.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ export interface OptionProps
*/
id?: string;
/** The option's visible text, displayed within `<Textbox>` of `<Select>`, and used for filtering */
text: string;
/** Optional: alternative rendered content, displayed within `<SelectList>` of `<Select>` (eg: an icon, an image, etc) */
text?: string;
/** Alternative rendered content, displayed within `<SelectList>` of `<Select>` (eg: an icon, an image, etc) */
children?: React.ReactNode;
/** The option's invisible internal value */
value: string | Record<string, unknown>;
/** The option's invisible internal value, if this is not passed the option will not be treated as interactive or selectable */
value?: string | Record<string, unknown>;
/** MultiSelect only - custom Pill border color - provide any color from palette or any valid css color value. */
borderColor?: string;
/** MultiSelect only - fill Pill background with color */
Expand Down Expand Up @@ -69,12 +69,12 @@ const Option = React.forwardRef(
let isSelected = selectListContext.currentOptionsListIndex === index;
const internalIdRef = useRef(id || guid());

if (selectListContext.multiselectValues) {
if (selectListContext.multiselectValues && value) {
isSelected = selectListContext.multiselectValues.includes(value);
}

function handleClick() {
if (disabled) {
if (disabled || !value) {
return;
}
if (!onClick) {
Expand All @@ -97,6 +97,7 @@ const Option = React.forwardRef(
role="option"
hidden={hidden}
style={style}
isInteractive={!!value}
{...{ ...rest, fill: undefined }}
data-component="option"
>
Expand Down
19 changes: 11 additions & 8 deletions src/components/select/option/option.style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import { OptionProps } from ".";
interface StyledOptionProps extends Pick<OptionProps, "id"> {
isHighlighted?: boolean;
isDisabled?: boolean;
isInteractive: boolean;
}

const StyledOption = styled.li<StyledOptionProps>`
cursor: pointer;
box-sizing: border-box;
line-height: 16px;
padding: 12px 16px;
Expand All @@ -18,18 +18,21 @@ const StyledOption = styled.li<StyledOptionProps>`
left: 0;
width: 100%;
${({ isHighlighted }) =>
isHighlighted &&
${({ isInteractive, isHighlighted }) =>
isInteractive &&
css`
background-color: var(--colorsUtilityMajor200);
cursor: pointer;
:hover {
background-color: var(--colorsUtilityMajor100);
}
${isHighlighted &&
css`
background-color: var(--colorsUtilityMajor200);
`}
`}
${({ hidden }) => hidden && "display: none;"}
:hover {
background-color: var(--colorsUtilityMajor100);
}
${({ isDisabled }) =>
isDisabled &&
css`
Expand Down
26 changes: 26 additions & 0 deletions src/components/select/option/option.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@ test("should set `id` on the element when passed", () => {
);
});

test("should not render with cursor style pointer when no value or text is passed", () => {
render(
<ul>
<Option>Foo</Option>
</ul>
);

expect(screen.getByRole("option")).not.toHaveStyle("cursor: pointer");
});

describe("when `disabled` prop is set", () => {
it("should have expected style", () => {
render(
Expand Down Expand Up @@ -203,6 +213,22 @@ describe("when `disabled` prop is not set", () => {
value: "baz",
});
});

it("should not call onClick and onSelect if both are passed but no value is set", async () => {
const user = userEvent.setup();
const onSelect = jest.fn();
const onClick = jest.fn();
const props = { text: "foo", onSelect, onClick };
render(
<ul>
<Option {...props}>Foo</Option>
</ul>
);
await user.click(screen.getByRole("option"));

expect(onSelect).not.toHaveBeenCalled();
expect(onClick).not.toHaveBeenCalled();
});
});

describe("when the `multiSelectValues` list is passed via context", () => {
Expand Down
49 changes: 49 additions & 0 deletions src/components/select/simple-select/components.test-pw.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -629,3 +629,52 @@ export const ListWidth = ({
</Box>
);
};

export const ComplexCustomChildren = () => {
return (
<Box height={220}>
<Select
mb={0}
key="key"
id="id"
label="Select"
aria-label="aria label"
name="name"
value="value"
isLoading={false}
readOnly={false}
placeholder="placeholder"
onChange={() => {}}
onOpen={() => {}}
onListScrollBottom={() => {}}
>
<Option>
<Box
width="100%"
display="flex"
alignItems="center"
justifyContent="center"
mt={2}
mb={3}
flexDirection="column"
as="span"
>
<Box display="flex" mx={2}>
<Icon type="error" color="errorRed" />
<Box ml={1} width="100%">
<Box mb={1}>
<Typography variant="b" color="errorRed">
Something went wrong
</Typography>
</Box>
<Typography variant="p" color="errorRed" mb={0}>
We couldn't load the data. Please try again later.
</Typography>
</Box>
</Box>
</Box>
</Option>
</Select>
</Box>
);
};
48 changes: 48 additions & 0 deletions src/components/select/simple-select/simple-select-test.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -663,3 +663,51 @@ export const SimpleSelectWithTruncatedText = () => {
</Select>
);
};

export const ComplexCustomChildren = () => {
return (
<Box height={220}>
<Select
mb={0}
key="key"
id="id"
label="Select"
aria-label="aria label"
name="name"
value="value"
isLoading={false}
readOnly={false}
placeholder="placeholder"
onChange={() => {}}
onOpen={() => {}}
onListScrollBottom={() => {}}
>
<Option>
<Box
width="100%"
display="flex"
alignItems="center"
justifyContent="center"
mt={2}
mb={3}
flexDirection="column"
>
<Box display="flex" mx={2}>
<Icon type="error" color="errorRed" />
<Box ml={1} width="100%">
<Box mb={1}>
<Typography variant="b" color="errorRed">
Something went wrong
</Typography>
</Box>
<Typography variant="p" color="errorRed" mb={0}>
We couldn't load the data. Please try again later.
</Typography>
</Box>
</Box>
</Box>
</Option>
</Select>
</Box>
);
};
4 changes: 4 additions & 0 deletions src/components/select/simple-select/simple-select.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ This prop will be called every time a user scrolls to the bottom of the list.

### Custom Option content

It is also possible to render non-interactive content within the `Option` component.
This can be achieved by passing `children` with no `value` or `text` props set. However,
we recommend checking that there are no accessibility issues with this approach before using it.

<Canvas of={SimpleSelectStories.CustomOptionChildren} />

### With multiple columns
Expand Down
21 changes: 21 additions & 0 deletions src/components/select/simple-select/simple-select.pw.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
SimpleSelectControlled,
WithObjectAsValue,
ListWidth,
ComplexCustomChildren,
} from "./components.test-pw";
import {
commonDataElementInputPreview,
Expand Down Expand Up @@ -1131,6 +1132,26 @@ test.describe("SimpleSelect component", () => {
await expect(selectInput(page)).toHaveCSS("border-radius", "4px");
await expect(selectListWrapper(page)).toHaveCSS("border-radius", "4px");
});

test("should display the custom option in full when list is opened", async ({
mount,
page,
}) => {
await mount(<ComplexCustomChildren />);

await selectText(page).click();
const customOptionContent = selectListCustomChild(page, 1).nth(0);
const wrapper = selectListWrapper(page);
const customOptionHeight = await selectList(page)
.locator("li")
.evaluate((element) => element.getBoundingClientRect().height);
const wrapperHeight = await wrapper.evaluate(
(element) => element.getBoundingClientRect().height
);

await expect(customOptionContent).toBeVisible();
expect(wrapperHeight).toBe(customOptionHeight);
});
});

test.describe("Check height of Select list when opened", () => {
Expand Down

0 comments on commit fb37215

Please sign in to comment.