fix(select): prevent deprecation warning about uncontrolled textbox f…

…rom being fired Ensures that Textbox used in Select components is used in a controlled manner to prevent the deprecation warnings from being fired fix #6883
Sage · Oct 3, 2024 · 6de179c · 6de179c
1 parent f73d0d5
commit 6de179c
Show file tree

Hide file tree

Showing 9 changed files with 132 additions and 84 deletions.
diff --git a/src/components/select/__internal__/select-textbox/select-textbox.component.tsx b/src/components/select/__internal__/select-textbox/select-textbox.component.tsx
@@ -1,6 +1,5 @@
 import React from "react";
 
-import { CustomSelectChangeEvent } from "../../simple-select/simple-select.component";
 import { SelectTextboxContext } from "./select-textbox.context";
 import {
   StyledSelectText,
@@ -40,10 +39,6 @@ export interface FormInputPropTypes
   name?: string;
   /** Specify a callback triggered on blur */
   onBlur?: (ev: React.FocusEvent<HTMLInputElement>) => void;
-  /** Specify a callback triggered on change */
-  onChange?: (
-    ev: CustomSelectChangeEvent | React.ChangeEvent<HTMLInputElement>
-  ) => void;
   /** Specify a callback triggered on click */
   onClick?: (ev: React.MouseEvent<HTMLInputElement>) => void;
   /** Specify a callback triggered on focus */
@@ -114,7 +109,6 @@ const SelectTextbox = React.forwardRef(
       onClick,
       onFocus,
       onBlur,
-      onChange,
       formattedValue = "",
       selectedValue,
       required,
@@ -182,7 +176,8 @@ const SelectTextbox = React.forwardRef(
           inputIcon="dropdown"
           autoComplete="off"
           size={size}
-          onChange={onChange}
+          // prevent uncontrolled warning being fired
+          onChange={() => {}}
           formattedValue={formattedValue}
           value={
             hasStringValue ? (selectedValue as string | string[]) : undefined

diff --git a/src/components/select/__internal__/select-textbox/select-textbox.test.tsx b/src/components/select/__internal__/select-textbox/select-textbox.test.tsx
@@ -5,31 +5,31 @@ import userEvent from "@testing-library/user-event";
 import SelectTextbox from ".";
 
 test("renders as a combobox", () => {
-  render(<SelectTextbox label="Select Colour" onChange={() => {}} />);
+  render(<SelectTextbox label="Select Colour" />);
 
   expect(
     screen.getByRole("combobox", { name: /Select Colour/i })
   ).toBeVisible();
 });
 
 test("renders as a readonly textbox if readOnly prop is true", () => {
-  render(<SelectTextbox label="Select Colour" onChange={() => {}} readOnly />);
+  render(<SelectTextbox label="Select Colour" readOnly />);
 
   const input = screen.getByRole("textbox", { name: /Select Colour/i });
   expect(input).toBeVisible();
   expect(input).toHaveAttribute("readonly");
 });
 
 test("renders as a disabled combobox if disabled prop is true", () => {
-  render(<SelectTextbox label="Select Colour" onChange={() => {}} disabled />);
+  render(<SelectTextbox label="Select Colour" disabled />);
 
   const input = screen.getByRole("combobox", { name: /Select Colour/i });
   expect(input).toBeVisible();
   expect(input).toBeDisabled();
 });
 
 test("combobox has correct accessible name when ariaLabel prop is provided", () => {
-  render(<SelectTextbox onChange={() => {}} ariaLabel="Label" />);
+  render(<SelectTextbox ariaLabel="Label" />);
 
   expect(screen.getByRole("combobox")).toHaveAccessibleName("Label");
 });
@@ -38,7 +38,7 @@ test("combobox has correct accessible name when ariaLabelledBy prop is provided"
   render(
     <>
       <h3 id="label">Label</h3>
-      <SelectTextbox onChange={() => {}} ariaLabelledby="label" />
+      <SelectTextbox ariaLabelledby="label" />
     </>
   );
 
@@ -49,24 +49,22 @@ test("combobox has correct accessible name when accessibilityLabelId prop is pro
   render(
     <>
       <h3 id="label">Label</h3>
-      <SelectTextbox onChange={() => {}} accessibilityLabelId="label" />
+      <SelectTextbox accessibilityLabelId="label" />
     </>
   );
 
   expect(screen.getByRole("combobox")).toHaveAccessibleName("Label");
 });
 
 test("renders dropdown icon", () => {
-  render(<SelectTextbox onChange={() => {}} />);
+  render(<SelectTextbox />);
 
   expect(screen.getByTestId("icon")).toHaveAttribute("type", "dropdown");
 });
 
 test("calls onFocus callback when combobox is focused", () => {
   const onFocus = jest.fn();
-  render(
-    <SelectTextbox label="Textbox" onChange={() => {}} onFocus={onFocus} />
-  );
+  render(<SelectTextbox label="Textbox" onFocus={onFocus} />);
 
   screen.getByRole("combobox", { name: "Textbox" }).focus();
 
@@ -75,14 +73,7 @@ test("calls onFocus callback when combobox is focused", () => {
 
 test("does not call onFocus callback when combobox is disabled", () => {
   const onFocus = jest.fn();
-  render(
-    <SelectTextbox
-      label="Textbox"
-      onChange={() => {}}
-      onFocus={onFocus}
-      disabled
-    />
-  );
+  render(<SelectTextbox label="Textbox" onFocus={onFocus} disabled />);
 
   screen.getByRole("combobox", { name: "Textbox" }).focus();
 
@@ -91,7 +82,7 @@ test("does not call onFocus callback when combobox is disabled", () => {
 
 test("calls onBlur callback when combobox is blurred", () => {
   const onBlur = jest.fn();
-  render(<SelectTextbox label="Textbox" onChange={() => {}} onBlur={onBlur} />);
+  render(<SelectTextbox label="Textbox" onBlur={onBlur} />);
 
   const combobox = screen.getByRole("combobox", { name: "Textbox" });
   combobox.focus();
@@ -101,7 +92,7 @@ test("calls onBlur callback when combobox is blurred", () => {
 });
 
 test("combobox has placeholder text when it has no value and hasTextCursor prop is true", () => {
-  render(<SelectTextbox onChange={() => {}} hasTextCursor />);
+  render(<SelectTextbox hasTextCursor />);
 
   expect(screen.getByRole("combobox")).toHaveAttribute(
     "placeholder",
@@ -110,23 +101,14 @@ test("combobox has placeholder text when it has no value and hasTextCursor prop
 });
 
 test("combobox has custom placeholder text when it has no value, hasTextCursor prop is true and a custom placeholder is provided", () => {
-  render(
-    <SelectTextbox onChange={() => {}} hasTextCursor placeholder="foobar" />
-  );
+  render(<SelectTextbox hasTextCursor placeholder="foobar" />);
 
   expect(screen.getByRole("combobox")).toHaveAttribute("placeholder", "foobar");
 });
 
 test("does not call onFocus callback when textbox is read only", () => {
   const onFocus = jest.fn();
-  render(
-    <SelectTextbox
-      label="Textbox"
-      onChange={() => {}}
-      onFocus={onFocus}
-      readOnly
-    />
-  );
+  render(<SelectTextbox label="Textbox" onFocus={onFocus} readOnly />);
 
   screen.getByRole("textbox", { name: "Textbox" }).focus();
 
@@ -140,7 +122,6 @@ describe("when hasTextCursor prop is false", () => {
         label="Textbox"
         formattedValue="foo"
         hasTextCursor={false}
-        onChange={() => {}}
       />
     );
 
@@ -151,32 +132,22 @@ describe("when hasTextCursor prop is false", () => {
   });
 
   it("displays the placeholder text in an overlay when the combobox has no value", () => {
-    render(
-      <SelectTextbox
-        placeholder="foobaz"
-        hasTextCursor={false}
-        onChange={() => {}}
-      />
-    );
+    render(<SelectTextbox placeholder="foobaz" hasTextCursor={false} />);
 
     expect(screen.getByRole("combobox")).not.toHaveTextContent("foobaz");
     expect(screen.getByText("foobaz")).toBeVisible();
   });
 
   it("renders combobox without autocomplete", () => {
-    render(<SelectTextbox label="Textbox" onChange={() => {}} />);
+    render(<SelectTextbox label="Textbox" />);
 
     const combobox = screen.getByRole("combobox", { name: "Textbox" });
     expect(combobox).toHaveAttribute("autocomplete", "off");
   });
 
   it("hides the combobox overlay from assistive technologies", () => {
     render(
-      <SelectTextbox
-        formattedValue="You can't see me"
-        hasTextCursor={false}
-        onChange={() => {}}
-      />
+      <SelectTextbox formattedValue="You can't see me" hasTextCursor={false} />
     );
 
     expect(screen.getByTestId("select-text")).toHaveAttribute(
@@ -189,12 +160,7 @@ describe("when hasTextCursor prop is false", () => {
     const onClick = jest.fn();
     const user = userEvent.setup();
     render(
-      <SelectTextbox
-        label="Textbox"
-        onChange={() => {}}
-        onClick={onClick}
-        hasTextCursor={false}
-      />
+      <SelectTextbox label="Textbox" onClick={onClick} hasTextCursor={false} />
     );
 
     await user.click(screen.getByText("Please Select..."));
@@ -203,13 +169,7 @@ describe("when hasTextCursor prop is false", () => {
   });
 
   it("truncates the displayed text of the selected option with ellipsis", () => {
-    render(
-      <SelectTextbox
-        formattedValue="foo"
-        hasTextCursor={false}
-        onChange={() => {}}
-      />
-    );
+    render(<SelectTextbox formattedValue="foo" hasTextCursor={false} />);
 
     expect(screen.getByText("foo")).toHaveStyle({
       whiteSpace: "nowrap",
@@ -220,12 +180,7 @@ describe("when hasTextCursor prop is false", () => {
 
   it("applies correct styles when disabled", () => {
     render(
-      <SelectTextbox
-        disabled
-        formattedValue="foo"
-        hasTextCursor={false}
-        onChange={() => {}}
-      />
+      <SelectTextbox disabled formattedValue="foo" hasTextCursor={false} />
     );
 
     expect(screen.getByTestId("select-text")).toHaveStyle({
@@ -237,12 +192,7 @@ describe("when hasTextCursor prop is false", () => {
 
   it("applies correct styles when read-only", () => {
     render(
-      <SelectTextbox
-        readOnly
-        formattedValue="foo"
-        hasTextCursor={false}
-        onChange={() => {}}
-      />
+      <SelectTextbox readOnly formattedValue="foo" hasTextCursor={false} />
     );
 
     expect(screen.getByTestId("select-text")).toHaveStyle({
@@ -254,12 +204,7 @@ describe("when hasTextCursor prop is false", () => {
 
   it("applies correct styles when transparent", () => {
     render(
-      <SelectTextbox
-        transparent
-        formattedValue="foo"
-        hasTextCursor={false}
-        onChange={() => {}}
-      />
+      <SelectTextbox transparent formattedValue="foo" hasTextCursor={false} />
     );
 
     expect(screen.getByTestId("select-text")).toHaveStyle({

diff --git a/src/components/select/filterable-select/filterable-select.component.tsx b/src/components/select/filterable-select/filterable-select.component.tsx
@@ -90,6 +90,10 @@ export interface FilterableSelectProps
   isOptional?: boolean;
   /** Flag to configure component as mandatory */
   required?: boolean;
+  /** Specify a callback triggered on change */
+  onChange?: (
+    ev: CustomSelectChangeEvent | React.ChangeEvent<HTMLInputElement>
+  ) => void;
 }
 
 export const FilterableSelect = React.forwardRef<

diff --git a/src/components/select/filterable-select/filterable-select.test.tsx b/src/components/select/filterable-select/filterable-select.test.tsx
@@ -2,6 +2,7 @@ import React from "react";
 import { render, screen } from "@testing-library/react";
 import FilterableSelect from ".";
 import Option from "../option";
+import Logger from "../../../__internal__/utils/logger";
 
 test("renders combobox without text overlay", () => {
   render(
@@ -81,3 +82,34 @@ test("combobox has correct accessible name when aria-labelledby prop is provided
 
   expect(screen.getByRole("combobox")).toHaveAccessibleName("My Select");
 });
+
+test("should not display deprecation about uncontrolled Textbox when parent component is controlled", () => {
+  const loggerSpy = jest.spyOn(Logger, "deprecate");
+  render(
+    <FilterableSelect
+      label="Colour"
+      onChange={() => {}}
+      value="1"
+      placeholder="Select a colour"
+    >
+      <Option text="Amber" value="1" />
+    </FilterableSelect>
+  );
+
+  expect(loggerSpy).not.toHaveBeenCalled();
+  loggerSpy.mockClear();
+});
+
+test("should not display deprecation about uncontrolled Textbox when parent component is not controlled", () => {
+  const loggerSpy = jest.spyOn(Logger, "deprecate");
+  render(
+    <FilterableSelect label="Colour" placeholder="Select a colour">
+      <Option text="Amber" value="1" />
+    </FilterableSelect>
+  );
+
+  expect(loggerSpy).not.toHaveBeenCalledWith(
+    "Uncontrolled behaviour in `Textbox` is deprecated and support will soon be removed. Please make sure all your inputs are controlled."
+  );
+  loggerSpy.mockClear();
+});
diff --git a/src/components/select/multi-select/multi-select.component.tsx b/src/components/select/multi-select/multi-select.component.tsx
@@ -91,6 +91,10 @@ export interface MultiSelectProps
   virtualScrollOverscan?: number;
   /** Flag to configure component as optional. */
   isOptional?: boolean;
+  /** Specify a callback triggered on change */
+  onChange?: (
+    ev: CustomSelectChangeEvent | React.ChangeEvent<HTMLInputElement>
+  ) => void;
 }
 
 export const MultiSelect = React.forwardRef<HTMLInputElement, MultiSelectProps>(

diff --git a/src/components/select/multi-select/multi-select.test.tsx b/src/components/select/multi-select/multi-select.test.tsx
@@ -2,6 +2,7 @@ import React from "react";
 import { render, screen } from "@testing-library/react";
 import MultiSelect from ".";
 import Option from "../option";
+import Logger from "../../../__internal__/utils/logger";
 
 test("renders combobox without text overlay", () => {
   render(
@@ -71,3 +72,34 @@ test("combobox has correct accessible name when aria-labelledby prop is provided
 
   expect(screen.getByRole("combobox")).toHaveAccessibleName("My Select");
 });
+
+test("should not display deprecation about uncontrolled Textbox when parent component is controlled", () => {
+  const loggerSpy = jest.spyOn(Logger, "deprecate");
+  render(
+    <MultiSelect
+      label="Colour"
+      onChange={() => {}}
+      value={["1"]}
+      placeholder="Select a colour"
+    >
+      <Option text="Amber" value="1" />
+    </MultiSelect>
+  );
+
+  expect(loggerSpy).not.toHaveBeenCalled();
+  loggerSpy.mockClear();
+});
+
+test("should not display deprecation about uncontrolled Textbox when parent component is not controlled", () => {
+  const loggerSpy = jest.spyOn(Logger, "deprecate");
+  render(
+    <MultiSelect label="Colour" placeholder="Select a colour">
+      <Option text="Amber" value="1" />
+    </MultiSelect>
+  );
+
+  expect(loggerSpy).not.toHaveBeenCalledWith(
+    "Uncontrolled behaviour in `Textbox` is deprecated and support will soon be removed. Please make sure all your inputs are controlled."
+  );
+  loggerSpy.mockClear();
+});