From 3b6d900c212df12801228ebc9b4e7a221770a552 Mon Sep 17 00:00:00 2001
From: Haider Alshamma <haidera@nulogy.com>
Date: Wed, 16 Oct 2024 11:13:12 -0400
Subject: [PATCH] fix: make the icons and indicators in the select accessible
 (#1462)

---
 cypress/e2e/components/AsyncSelect.spec.ts  | 24 +++---
 src/AsyncSelect/AsyncSelect.story.tsx       | 91 +++++++++------------
 src/AsyncSelect/fixtures.ts                 | 54 ++++++++++++
 src/Select/Select.story.tsx                 | 12 +--
 src/Select/customReactSelectStyles.spec.tsx | 33 ++++----
 src/Select/customReactSelectStyles.tsx      | 30 +++----
 src/utils/story/simulatedAPIRequest.ts      | 19 -----
 src/utils/story/simulatedAPIRequests.ts     | 17 ++++
 yarn.lock                                   | 18 +---
 9 files changed, 163 insertions(+), 135 deletions(-)
 create mode 100644 src/AsyncSelect/fixtures.ts
 delete mode 100644 src/utils/story/simulatedAPIRequest.ts
 create mode 100644 src/utils/story/simulatedAPIRequests.ts

diff --git a/cypress/e2e/components/AsyncSelect.spec.ts b/cypress/e2e/components/AsyncSelect.spec.ts
index 674d0932c..bb360152a 100644
--- a/cypress/e2e/components/AsyncSelect.spec.ts
+++ b/cypress/e2e/components/AsyncSelect.spec.ts
@@ -14,41 +14,41 @@ describe("AsyncSelect", () => {
     it("can select multiple values", () => {
       getMultiselect().click();
 
-      cy.focused().type("cana");
-      assertDropDownIsOpen().contains("Canada");
+      cy.focused().type("on");
+      assertDropDownIsOpen().contains("Ontario");
       cy.focused().type("{enter}");
 
-      cy.focused().type("mex");
-      assertDropDownIsOpen().contains("Mexico");
+      cy.focused().type("qu");
+      assertDropDownIsOpen().contains("Quebec");
       cy.focused().type("{enter}");
 
-      getMultiselect().contains("Canada");
-      getMultiselect().contains("Mexico");
+      getMultiselect().contains("Ontario");
+      getMultiselect().contains("Quebec");
     });
 
     describe("clears selected values", () => {
       it("clears all multiselect values", () => {
         getMultiselect().click();
 
-        cy.focused().type("cana");
-        assertDropDownIsOpen().contains("Canada");
+        cy.focused().type("on");
+        assertDropDownIsOpen().contains("Ontario");
         cy.focused().type("{enter}");
 
         getClearButton().click();
 
-        getMultiselect().contains("Select countries");
+        getMultiselect().contains("Enter a province");
       });
 
       it("clears single-select values", () => {
         getSelectComponent().click();
 
-        cy.focused().type("cana");
-        assertDropDownIsOpen().contains("Canada");
+        cy.focused().type("on");
+        assertDropDownIsOpen().contains("Ontario");
         cy.focused().type("{enter}");
 
         getClearButton().click();
 
-        getMultiselect().contains("Select countries");
+        getMultiselect().contains("Enter a province");
       });
     });
 
diff --git a/src/AsyncSelect/AsyncSelect.story.tsx b/src/AsyncSelect/AsyncSelect.story.tsx
index 37e29a499..662528ed8 100644
--- a/src/AsyncSelect/AsyncSelect.story.tsx
+++ b/src/AsyncSelect/AsyncSelect.story.tsx
@@ -2,31 +2,13 @@ import React, { useRef } from "react";
 import { action } from "@storybook/addon-actions";
 import { useState } from "react";
 import { AsyncSelect, Button } from "../index";
-import simulatedAPIRequest from "../utils/story/simulatedAPIRequest";
-
-const northAmericanCountries = [
-  {
-    value: "Canada",
-    label: "Canada",
-  },
-  {
-    value: "United States",
-    label: "United States",
-  },
-  {
-    value: "Mexico",
-    label: "Mexico",
-  },
-];
-
-const loadMatchingCountries = async (inputValue: string) => {
-  const data = await simulatedAPIRequest(inputValue, northAmericanCountries);
-  const results = await data.json();
-
-  return results.map(({ name }) => ({
-    label: name,
-    value: name,
-  }));
+import { filterOptions } from "../utils/story/simulatedAPIRequests";
+import { provinces } from "./fixtures";
+import { Flex } from "../Flex";
+
+const loadMatchingProvinces = async (inputValue: string) => {
+  const data = await filterOptions(inputValue, provinces);
+  return await data.json();
 };
 
 export default {
@@ -35,14 +17,14 @@ export default {
 
 export const Default = () => (
   <AsyncSelect
-    placeholder="Please select a country"
+    placeholder="Enter a province"
     onChange={action("selection changed")}
     onBlur={action("blurred")}
     className="Select"
     classNamePrefix="SelectTest"
-    labelText="Country"
+    labelText="Province"
     onInputChange={action("typed input value changed")}
-    loadOptions={loadMatchingCountries}
+    loadOptions={loadMatchingProvinces}
   />
 );
 
@@ -52,25 +34,25 @@ Default.story = {
 
 export const WithDefaultOptions = () => (
   <AsyncSelect
-    placeholder="Filter Countries"
+    placeholder="Enter a province"
     onChange={action("selection changed")}
     onBlur={action("blurred")}
     className="Select"
     classNamePrefix="SelectTest"
-    labelText="Country"
+    labelText="Province"
     onInputChange={action("typed input value changed")}
     isClearable
     defaultOptions={[
       {
-        value: "Canada",
-        label: "Canada",
+        value: "ON",
+        label: "Ontario",
       },
       {
-        value: "United States",
-        label: "United States",
+        value: "QC",
+        label: "Quebec",
       },
     ]}
-    loadOptions={loadMatchingCountries}
+    loadOptions={loadMatchingProvinces}
   />
 );
 
@@ -80,15 +62,15 @@ WithDefaultOptions.story = {
 
 export const WithADefaultValue = () => (
   <AsyncSelect
-    placeholder="Please select a country"
+    placeholder="Enter a province"
     onChange={action("selection changed")}
     onBlur={action("blurred")}
     className="Select"
     classNamePrefix="SelectTest"
-    labelText="Country"
-    defaultValue="Can"
+    labelText="Province"
+    defaultValue="Ontario"
     onInputChange={action("typed input value changed")}
-    loadOptions={loadMatchingCountries}
+    loadOptions={loadMatchingProvinces}
   />
 );
 
@@ -98,29 +80,29 @@ WithADefaultValue.story = {
 
 export const Multiselect = () => (
   <AsyncSelect
-    placeholder="Select countries"
+    placeholder="Enter a province"
     onChange={action("selection changed")}
     onBlur={action("blurred")}
     className="Select"
     classNamePrefix="SelectTest"
-    labelText="Countries"
+    labelText="Provinces"
     multiselect
     onInputChange={action("typed input value changed")}
-    loadOptions={loadMatchingCountries}
+    loadOptions={loadMatchingProvinces}
   />
 );
 
 export const WithAClearButton = () => (
   <AsyncSelect
-    placeholder="Select countries"
+    placeholder="Enter a province"
     onChange={action("selection changed")}
     onBlur={action("blurred")}
     className="Select"
     classNamePrefix="SelectTest"
-    labelText="Countries"
+    labelText="Provinces"
     isClearable
     onInputChange={action("typed input value changed")}
-    loadOptions={loadMatchingCountries}
+    loadOptions={loadMatchingProvinces}
   />
 );
 
@@ -131,21 +113,21 @@ export const UsingRefToControlFocus = () => {
   };
 
   return (
-    <>
+    <Flex gap="x2" flexDirection="column">
       <AsyncSelect
         ref={ref}
-        placeholder="Please select a country"
+        placeholder="Enter a province"
         onChange={action("selection changed")}
         onBlur={action("blurred")}
         className="Select"
         classNamePrefix="SelectTest"
-        labelText="Country"
-        defaultValue="Can"
+        labelText="Province"
+        defaultValue="Ontario"
         onInputChange={action("typed input value changed")}
-        loadOptions={loadMatchingCountries}
+        loadOptions={loadMatchingProvinces}
       />
       <Button onClick={handleClick}>Focus the Input</Button>
-    </>
+    </Flex>
   );
 };
 
@@ -164,12 +146,13 @@ export const Controlled = () => {
   };
 
   return (
-    <>
-      <AsyncSelect onChange={handleChange} value={value} labelText="Country" loadOptions={loadMatchingCountries} />
+    <Flex gap="x2" flexDirection="column">
+      <AsyncSelect onChange={handleChange} value={value} labelText="Province" loadOptions={loadMatchingProvinces} />
       <Button onClick={handleClear}>Clear</Button>
-    </>
+    </Flex>
   );
 };
+
 Controlled.story = {
   name: "controlled",
 };
diff --git a/src/AsyncSelect/fixtures.ts b/src/AsyncSelect/fixtures.ts
new file mode 100644
index 000000000..7fe936fc2
--- /dev/null
+++ b/src/AsyncSelect/fixtures.ts
@@ -0,0 +1,54 @@
+export const provinces = [
+  {
+    label: "Alberta",
+    value: "AB",
+  },
+  {
+    label: "British Columbia",
+    value: "BC",
+  },
+  {
+    label: "Manitoba",
+    value: "MB",
+  },
+  {
+    label: "New Brunswick",
+    value: "NB",
+  },
+  {
+    label: "Newfoundland and Labrador",
+    value: "NL",
+  },
+  {
+    label: "Northwest Territories",
+    value: "NT",
+  },
+  {
+    label: "Nova Scotia",
+    value: "NS",
+  },
+  {
+    label: "Nunavut",
+    value: "NU",
+  },
+  {
+    label: "Ontario",
+    value: "ON",
+  },
+  {
+    label: "Prince Edward Island",
+    value: "PE",
+  },
+  {
+    label: "Quebec",
+    value: "QC",
+  },
+  {
+    label: "Saskatchewan",
+    value: "SK",
+  },
+  {
+    label: "Yukon Territory",
+    value: "YT",
+  },
+];
diff --git a/src/Select/Select.story.tsx b/src/Select/Select.story.tsx
index 802904603..469de13b6 100644
--- a/src/Select/Select.story.tsx
+++ b/src/Select/Select.story.tsx
@@ -479,7 +479,7 @@ WithCloseMenuOnSelectTurnedOff.story = {
 };
 
 export const TestMultiselectOverflow = () => (
-  <>
+  <Flex gap="x2" flexDirection="column">
     <Select
       defaultValue={["accepted", "assigned"]}
       noOptionsMessage={() => "No options"}
@@ -517,7 +517,7 @@ export const TestMultiselectOverflow = () => (
         onInputChange={action("typed input value changed")}
       />
     </Box>
-  </>
+  </Flex>
 );
 
 TestMultiselectOverflow.story = {
@@ -551,10 +551,10 @@ WithFixedPositioning.story = {
 };
 
 export const WithFetchedOptions = () => (
-  <Box style={{ width: "300px" }}>
+  <Flex flexDirection="column" gap="x2" width="300px">
     <SelectWithManyOptions labelText="Select from many options:" />
     <SelectWithManyOptions multiselect labelText="Multiselect many options:" />
-  </Box>
+  </Flex>
 );
 
 export const WithCustomOptionComponent = () => {
@@ -609,7 +609,7 @@ export const UsingRefToControlFocus = () => {
   };
 
   return (
-    <>
+    <Flex flexDirection="column" gap="x2">
       <Select
         defaultValue={["accepted"]}
         noOptionsMessage={() => "No options"}
@@ -621,7 +621,7 @@ export const UsingRefToControlFocus = () => {
         menuPosition="fixed"
       />
       <Button onClick={handleClick}>Focus the Input</Button>
-    </>
+    </Flex>
   );
 };
 
diff --git a/src/Select/customReactSelectStyles.spec.tsx b/src/Select/customReactSelectStyles.spec.tsx
index aabcce6aa..91076cc80 100644
--- a/src/Select/customReactSelectStyles.spec.tsx
+++ b/src/Select/customReactSelectStyles.spec.tsx
@@ -3,22 +3,23 @@ import { getControlBorderRadius, getMenuBorderRadius, showIndicatorSeparator } f
 
 describe("custom react-select styles", () => {
   describe("showIndicatorSeparator", () => {
-    test.each`
-      isMulti  | hasValue | hasDefaultOptions | expected
-      ${true}  | ${true}  | ${true}           | ${true}
-      ${true}  | ${false} | ${false}          | ${false}
-      ${false} | ${true}  | ${false}          | ${false}
-      ${false} | ${false} | ${true}           | ${false}
-      ${true}  | ${true}  | ${false}          | ${false}
-      ${false} | ${true}  | ${true}           | ${false}
-      ${true}  | ${false} | ${true}           | ${false}
-      ${false} | ${false} | ${false}          | ${false}
-    `(
-      "returns $expected when isMulti is $isMulti, hasValue is $hasValue, and hasDefaultOptions is $hasDefaultOptions",
-      ({ isMulti, hasValue, hasDefaultOptions, expected }) => {
-        expect(showIndicatorSeparator({ isMulti, hasValue, hasDefaultOptions })).toBe(expected);
-      }
-    );
+    it("should not show the indicator separator when the select has no value", () => {
+      const result = showIndicatorSeparator({ hasValue: false, isClearable: true, isMulti: true });
+
+      expect(result).toEqual(false);
+    });
+
+    it("should show the indicator separator when the select allows multiple selections", () => {
+      const result = showIndicatorSeparator({ hasValue: true, isClearable: false, isMulti: true });
+
+      expect(result).toEqual(true);
+    });
+
+    it("should show the indicator separator when the select is clearable", () => {
+      const result = showIndicatorSeparator({ hasValue: true, isClearable: false, isMulti: true });
+
+      expect(result).toEqual(true);
+    });
   });
 
   describe("getControlBorderRadius", () => {
diff --git a/src/Select/customReactSelectStyles.tsx b/src/Select/customReactSelectStyles.tsx
index fbf861aac..7a907831a 100644
--- a/src/Select/customReactSelectStyles.tsx
+++ b/src/Select/customReactSelectStyles.tsx
@@ -77,12 +77,13 @@ type SizeConfig = {
   [key in ComponentSize]: CSSObject;
 };
 
-export function stylesForSize(config: SizeConfig, size: ComponentSize) {
+export function stylesForSize(config: SizeConfig, size: ComponentSize = "medium") {
   return config[size];
 }
 
-export const showIndicatorSeparator = ({ isMulti, hasValue, hasDefaultOptions }) =>
-  isMulti && hasValue && hasDefaultOptions;
+export function showIndicatorSeparator({ hasValue, isClearable, isMulti }) {
+  return hasValue && (isMulti || isClearable);
+}
 
 interface Args {
   theme: DefaultNDSThemeType;
@@ -96,6 +97,10 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
     option: () => ({
       height: 38,
     }),
+    clearIndicator: (provided) => ({
+      ...provided,
+      color: theme.colors.midGrey,
+    }),
     control: (provided, state) => ({
       ...provided,
       display: "flex",
@@ -107,7 +112,6 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
       lineHeight: theme.lineHeights.base,
       color: state.isDisabled ? transparentize(0.6667, theme.colors.black) : theme.colors.black,
       background: state.isDisabled ? theme.colors.whiteGrey : theme.colors.white,
-      border: `1px solid ${theme.colors.grey}`,
       borderColor: getBorderColor({
         errored: error,
         disabled: state.isDisabled,
@@ -136,7 +140,6 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
         menuPlacement: state.selectProps.menuPlacement,
         theme: theme,
       }),
-
       borderTopRightRadius: getControlBorderRadius({
         border: "top",
         isMenuOpen: state.selectProps.menuIsOpen,
@@ -151,7 +154,6 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
         menuPlacement: state.selectProps.menuPlacement,
         theme: theme,
       }),
-
       "&:hover, &:focus": {
         borderColor: getBorderColor({
           errored: error,
@@ -165,11 +167,11 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
     dropdownIndicator: (provided) => ({
       ...provided,
       ...(!hasDefaultOptions && { display: "none" }),
-      color: theme.colors.grey,
+      color: theme.colors.midGrey,
     }),
     indicatorsContainer: (provided) => ({
       ...provided,
-      color: theme.colors.grey,
+      color: theme.colors.midGrey,
     }),
     singleValue: (provided) => ({
       ...provided,
@@ -181,8 +183,8 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
     }),
     valueContainer: (provided, state) => ({
       ...provided,
-      display: "flex",
       padding: 0,
+      display: "flex",
       overflow: "auto",
       maxHeight: "150px",
       gap: theme.space.half,
@@ -194,8 +196,8 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
             gap: theme.space.x1,
           },
           medium: {
-            paddingTop: theme.space.half,
-            paddingBottom: theme.space.half,
+            paddingTop: theme.space.none,
+            paddingBottom: theme.space.none,
           },
         },
         size
@@ -313,9 +315,9 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
     indicatorSeparator: (provided, state) => ({
       ...provided,
       display: showIndicatorSeparator({
-        isMulti: state.isMulti,
         hasValue: state.hasValue,
-        hasDefaultOptions,
+        isClearable: state.selectProps.isClearable,
+        isMulti: state.isMulti,
       })
         ? "block"
         : "none",
@@ -329,7 +331,7 @@ const customStyles: <Option, IsMulti extends boolean, Group extends GroupBase<Op
         position: "absolute",
         top: "50%",
         transform: "translateY(-50%)",
-        color: state.isDisabled ? transparentize(0.6667, theme.colors.black) : "hsl(0,0%,50%)",
+        color: state.isDisabled ? transparentize(0.7, theme.colors.midGrey) : theme.colors.midGrey,
       };
     },
   };
diff --git a/src/utils/story/simulatedAPIRequest.ts b/src/utils/story/simulatedAPIRequest.ts
deleted file mode 100644
index d044f6694..000000000
--- a/src/utils/story/simulatedAPIRequest.ts
+++ /dev/null
@@ -1,19 +0,0 @@
-const simulatedAPIRequest = async (
-  inputValue: string,
-  data: { value: string; label: string }[],
-  milliseconds = 450
-): Promise<Response> => {
-  const country = data.find((country) => {
-    return country.value.toLowerCase().startsWith(inputValue.toLowerCase());
-  });
-
-  const responseBody = JSON.stringify([{ name: country.value }]);
-
-  return new Promise((resolve) => {
-    setTimeout(() => {
-      resolve(new Response(responseBody));
-    }, milliseconds);
-  });
-};
-
-export default simulatedAPIRequest;
diff --git a/src/utils/story/simulatedAPIRequests.ts b/src/utils/story/simulatedAPIRequests.ts
new file mode 100644
index 000000000..b9416318f
--- /dev/null
+++ b/src/utils/story/simulatedAPIRequests.ts
@@ -0,0 +1,17 @@
+export async function filterOptions(
+  inputValue: string,
+  options: { value: string; label: string }[],
+  simulatedNetworkDelayMs = 450
+): Promise<Response> {
+  const filteredOptions = options.filter((option) => {
+    return option.label.toLowerCase().startsWith(inputValue.toLowerCase());
+  });
+
+  const responseBody = JSON.stringify(filteredOptions);
+
+  return new Promise((resolve) => {
+    setTimeout(() => {
+      resolve(new Response(responseBody));
+    }, simulatedNetworkDelayMs);
+  });
+}
diff --git a/yarn.lock b/yarn.lock
index 37f16a6e0..1d16aa23e 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -7282,20 +7282,10 @@ camelize@^1.0.0:
   resolved "https://registry.yarnpkg.com/camelize/-/camelize-1.0.0.tgz#164a5483e630fa4321e5af07020e531831b2609b"
   integrity sha1-FkpUg+Yw+kMh5a8HAg5TGDGyYJs=
 
-caniuse-lite@^1.0.30001109, caniuse-lite@^1.0.30001125, caniuse-lite@^1.0.30001219, caniuse-lite@^1.0.30001280:
-  version "1.0.30001367"
-  resolved "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001367.tgz"
-  integrity sha512-XDgbeOHfifWV3GEES2B8rtsrADx4Jf+juKX2SICJcaUhjYBO3bR96kvEIHa15VU6ohtOhBZuPGGYGbXMRn0NCw==
-
-caniuse-lite@^1.0.30001449:
-  version "1.0.30001481"
-  resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001481.tgz#f58a717afe92f9e69d0e35ff64df596bfad93912"
-  integrity sha512-KCqHwRnaa1InZBtqXzP98LPg0ajCVujMKjqKDhZEthIpAsJl/YEIa3YvXjGXPVqzZVguccuu7ga9KOE1J9rKPQ==
-
-caniuse-lite@^1.0.30001587:
-  version "1.0.30001591"
-  resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001591.tgz#16745e50263edc9f395895a7cd468b9f3767cf33"
-  integrity sha512-PCzRMei/vXjJyL5mJtzNiUCKP59dm8Apqc3PH8gJkMnMXZGox93RbE76jHsmLwmIo6/3nsYIpJtx0O7u5PqFuQ==
+caniuse-lite@^1.0.30001109, caniuse-lite@^1.0.30001125, caniuse-lite@^1.0.30001219, caniuse-lite@^1.0.30001280, caniuse-lite@^1.0.30001449, caniuse-lite@^1.0.30001587:
+  version "1.0.30001668"
+  resolved "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30001668.tgz"
+  integrity sha512-nWLrdxqCdblixUO+27JtGJJE/txpJlyUy5YN1u53wLZkP0emYCo5zgS6QYft7VUYR42LGgi/S5hdLZTrnyIddw==
 
 cardinal@^2.1.1:
   version "2.1.1"