Skip to content

Commit

Permalink
fix(filterable-select): cannot backspace with object as values
Browse files Browse the repository at this point in the history
refactored the code to run setMatchingText only when the object change instead of on every render

fixes: #7064
  • Loading branch information
mihai-albu-sage committed Dec 18, 2024
1 parent e2c1dd9 commit eae40b5
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 3 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import areObjectsEqual from "./are-objects-equal";

const value1 = { id: "Amber", value: 1, text: "Amber" };
const value2 = { id: "Amber", value: 1, text: "Amber" };
const value3 = { id: "Black", value: 2, text: "Black" };
const value4 = { id: "Black", value: 2 };
const value5 = { ids: "Amber", values: 1, text: "Amber" };
const invalidParam = "notAnObject" as unknown as Record<string, unknown>;

test("returns true when the objects have the same keys and values", () => {
expect(areObjectsEqual(value1, value2)).toBe(true);
});

test("returns false when the objects have the same keys but not the same values", () => {
expect(areObjectsEqual(value1, value3)).toBe(false);
});

test("returns false when the objects do not have the same keys length", () => {
expect(areObjectsEqual(value1, value4)).toBe(false);
});

test("returns false when the objects have the same keys length but different key names", () => {
expect(areObjectsEqual(value1, value5)).toBe(false);
});

test("throws error if both parameters are not objects", () => {
expect(areObjectsEqual(value1, invalidParam)).toBe(false);
expect(areObjectsEqual(invalidParam, value1)).toBe(false);
});
32 changes: 32 additions & 0 deletions src/components/select/__internal__/utils/are-objects-equal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
export default function areObjectsEqual(
obj1: Record<string, unknown>,
obj2: Record<string, unknown>,
) {
// Check if both arguments are objects
if (
typeof obj1 !== "object" ||
typeof obj2 !== "object" ||
obj1 === null ||
obj2 === null
) {
return false;
}
// Get the keys of both objects
const keys1 = Object.keys(obj1);
const keys2 = Object.keys(obj2);
// If the number of keys is different, objects aren't equal
if (keys1.length !== keys2.length) {
return false;
} // Compare keys and values
for (const key of keys1) {
// Check if obj2 has the key and values are strictly equal
if (
!Object.prototype.hasOwnProperty.call(obj2, key) ||
obj1[key] !== obj2[key]
) {
return false;
}
}

return true;
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import SelectList, {
ListPlacement,
} from "../__internal__/select-list/select-list.component";
import isExpectedOption from "../__internal__/utils/is-expected-option";
import areObjectsEqual from "../__internal__/utils/are-objects-equal";
import isNavigationKey from "../__internal__/utils/is-navigation-key";
import Logger from "../../../__internal__/utils/logger";
import useStableCallback from "../../../hooks/__internal__/useStableCallback";
Expand Down Expand Up @@ -160,6 +161,7 @@ export const FilterableSelect = React.forwardRef<
const [selectedValue, setSelectedValue] = useState<
string | Record<string, unknown> | undefined
>(value || defaultValue || "");
const receivedValue = useRef(value);
const [highlightedValue, setHighlightedValue] = useState<
string | Record<string, unknown> | undefined
>("");
Expand Down Expand Up @@ -427,10 +429,28 @@ export const FilterableSelect = React.forwardRef<
);
}, [listActionButton, onListAction]);

const isFirstRender = useRef(true);

useEffect(() => {
if (isControlled.current) {
// when we render for the first time, we run setMatchingText to populate the input with the correct text
if (isFirstRender.current) {
setMatchingText(value);
}

if (isControlled.current) {
// when value is an object we should only run setMatchingText if the object changes between renders
if (
typeof value === "object" &&
typeof receivedValue.current === "object"
) {
if (!areObjectsEqual(value, receivedValue.current)) {
setMatchingText(value);
receivedValue.current = value;
}
} else {
setMatchingText(value);
}
}
// update text value only when children are changing
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [value, children]);
Expand All @@ -439,8 +459,6 @@ export const FilterableSelect = React.forwardRef<
onFilterChangeProp as (filterTextArg: unknown) => void,
);

const isFirstRender = useRef(true);

useEffect(() => {
if (onFilterChange && !isFirstRender.current) {
onFilterChange(filterText);
Expand Down
50 changes: 50 additions & 0 deletions src/components/select/filterable-select/filterable-select.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
screen,
waitFor,
within,
prettyDOM,
} from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { testStyledSystemMargin } from "../../../__spec_helper__/__internal__/test-utils";
Expand Down Expand Up @@ -54,6 +55,36 @@ const FilterableSelectWithState = ({
);
};

const FilterableSelectWithStateAndObjects = ({
label,
...props
}: Partial<FilterableSelectProps>) => {
const optionListValues = [
{ id: "Black", value: 1, text: "Black" },
{ id: "Blue", value: 2, text: "Blue" },
];

const [value, setValue] = useState<Record<string, unknown>>(
optionListValues[1],
);

function onChangeHandler(event: React.ChangeEvent<HTMLInputElement>) {
setValue(event.target.value as unknown as Record<string, unknown>);
}
return (
<FilterableSelect
label={label}
value={value}
onChange={onChangeHandler}
{...props}
>
{optionListValues.map((option) => (
<Option key={option.id} text={option.text} value={option} />
))}
</FilterableSelect>
);
};

test("should display a deprecation warning only once for all instances of component when they are uncontrolled", () => {
const loggerSpy = jest.spyOn(Logger, "deprecate");
render(
Expand Down Expand Up @@ -699,6 +730,25 @@ describe("when the input is focused", () => {
expect(screen.getByRole("combobox")).toHaveValue("red");
});

it("if the values are objects, deleting the last charcter with backspace works as expected and the correct match text is provided", async () => {
const user = userEvent.setup();

render(<FilterableSelectWithStateAndObjects />);

const input = screen.getByRole("combobox");

expect(input).toHaveValue("Blue");

await user.click(input);
await user.click(input);

await user.type(input, "{Backspace}");
expect(input).toHaveValue("Blu");

await user.type(input, "{Backspace}");
expect(input).toHaveValue("Black");
});

it("should display the list but not update the input value when the user types a character that does not match an option", async () => {
const user = userEvent.setup();
render(
Expand Down

0 comments on commit eae40b5

Please sign in to comment.