From fb372157e93cc171fa596ca9d74b299851db216c Mon Sep 17 00:00:00 2001
From: edleeks87 <edward.leeks@sage.com>
Date: Thu, 3 Oct 2024 14:59:42 +0100
Subject: [PATCH] fix(option): makes `value` and `text` props optional

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
---
 .../select-list/select-list.component.tsx     |  7 +--
 .../__internal__/utils/with-filter.hoc.tsx    |  4 +-
 .../select/option/option.component.tsx        | 13 ++---
 src/components/select/option/option.style.ts  | 19 ++++---
 src/components/select/option/option.test.tsx  | 26 ++++++++++
 .../simple-select/components.test-pw.tsx      | 49 +++++++++++++++++++
 .../simple-select-test.stories.tsx            | 48 ++++++++++++++++++
 .../select/simple-select/simple-select.mdx    |  4 ++
 .../select/simple-select/simple-select.pw.tsx | 21 ++++++++
 9 files changed, 173 insertions(+), 18 deletions(-)

diff --git a/src/components/select/__internal__/select-list/select-list.component.tsx b/src/components/select/__internal__/select-list/select-list.component.tsx
index 95e642e875..03bb666cc0 100644
--- a/src/components/select/__internal__/select-list/select-list.component.tsx
+++ b/src/components/select/__internal__/select-list/select-list.component.tsx
@@ -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]
     );
 
diff --git a/src/components/select/__internal__/utils/with-filter.hoc.tsx b/src/components/select/__internal__/utils/with-filter.hoc.tsx
index 1f162c349b..92c2d4a2fb 100644
--- a/src/components/select/__internal__/utils/with-filter.hoc.tsx
+++ b/src/components/select/__internal__/utils/with-filter.hoc.tsx
@@ -150,7 +150,9 @@ const withFilter = <T extends WrappedComponentProps>(
             }
 
             return (
-              <StyledOption>{noResultsMessage || noResultsText}</StyledOption>
+              <StyledOption isInteractive>
+                {noResultsMessage || noResultsText}
+              </StyledOption>
             );
           }
 
diff --git a/src/components/select/option/option.component.tsx b/src/components/select/option/option.component.tsx
index 4fa9679878..03ff613b77 100644
--- a/src/components/select/option/option.component.tsx
+++ b/src/components/select/option/option.component.tsx
@@ -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 */
@@ -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) {
@@ -97,6 +97,7 @@ const Option = React.forwardRef(
         role="option"
         hidden={hidden}
         style={style}
+        isInteractive={!!value}
         {...{ ...rest, fill: undefined }}
         data-component="option"
       >
diff --git a/src/components/select/option/option.style.ts b/src/components/select/option/option.style.ts
index c2d7e05802..6451a268e4 100644
--- a/src/components/select/option/option.style.ts
+++ b/src/components/select/option/option.style.ts
@@ -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;
@@ -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`
diff --git a/src/components/select/option/option.test.tsx b/src/components/select/option/option.test.tsx
index a55cfa64eb..f40d781e8d 100644
--- a/src/components/select/option/option.test.tsx
+++ b/src/components/select/option/option.test.tsx
@@ -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(
@@ -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", () => {
diff --git a/src/components/select/simple-select/components.test-pw.tsx b/src/components/select/simple-select/components.test-pw.tsx
index aad333cecb..27eea052b0 100644
--- a/src/components/select/simple-select/components.test-pw.tsx
+++ b/src/components/select/simple-select/components.test-pw.tsx
@@ -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>
+  );
+};
diff --git a/src/components/select/simple-select/simple-select-test.stories.tsx b/src/components/select/simple-select/simple-select-test.stories.tsx
index 4e2983e953..1a14a0376c 100644
--- a/src/components/select/simple-select/simple-select-test.stories.tsx
+++ b/src/components/select/simple-select/simple-select-test.stories.tsx
@@ -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>
+  );
+};
diff --git a/src/components/select/simple-select/simple-select.mdx b/src/components/select/simple-select/simple-select.mdx
index 9c6437eba8..9c6d0f19a4 100644
--- a/src/components/select/simple-select/simple-select.mdx
+++ b/src/components/select/simple-select/simple-select.mdx
@@ -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
diff --git a/src/components/select/simple-select/simple-select.pw.tsx b/src/components/select/simple-select/simple-select.pw.tsx
index 98e449fb87..0efb533986 100644
--- a/src/components/select/simple-select/simple-select.pw.tsx
+++ b/src/components/select/simple-select/simple-select.pw.tsx
@@ -19,6 +19,7 @@ import {
   SimpleSelectControlled,
   WithObjectAsValue,
   ListWidth,
+  ComplexCustomChildren,
 } from "./components.test-pw";
 import {
   commonDataElementInputPreview,
@@ -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", () => {