Skip to content

Commit

Permalink
Merge pull request #6859 from Sage/FE-6718-duplicate-option-text
Browse files Browse the repository at this point in the history
fix(multi-select): ensure unique keys when options passed object values with duplicate text property FE-6718
  • Loading branch information
edleeks87 committed Aug 2, 2024
2 parents 9d29132 + 1c38be2 commit e09d172
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 6 deletions.
30 changes: 30 additions & 0 deletions src/components/select/multi-select/components.test-pw.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -523,3 +523,33 @@ export const MultiSelectWithDisabledOption = () => {
</>
);
};

const optionListValues = [
{
id: "Amber",
value: 1,
text: "Black",
},
{
id: "Black",
value: 2,
text: "Black",
},
{
id: "Blue",
value: 3,
text: "Blue",
},
];

export const OptionsWithSameName = () => (
<MultiSelect
name="multi-options-with-same-name"
id="multi-options-with-same-name"
label="multi options with same name"
>
{optionListValues.map((option) => (
<Option key={option.id} text={option.text} value={option} />
))}
</MultiSelect>
);
33 changes: 33 additions & 0 deletions src/components/select/multi-select/multi-select-test.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -565,3 +565,36 @@ export const SingleOption = () => {
};

SingleOption.storyName = "Single Option";

const optionListValues = [
{
id: "Amber",
value: 1,
text: "Black",
},
{
id: "Black",
value: 2,
text: "Black",
},
{
id: "Blue",
value: 3,
text: "Blue",
},
];

export const OptionsWithSameName = () => {
return (
<MultiSelect
name="multi-options-with-same-name"
id="multi-options-with-same-name"
label="multi options with same name"
>
{optionListValues.map((option) => (
<Option key={option.id} text={option.text} value={option} />
))}
</MultiSelect>
);
};
OptionsWithSameName.storyName = "Options with same name";
13 changes: 7 additions & 6 deletions src/components/select/multi-select/multi-select.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ import Logger from "../../../__internal__/utils/logger";
import useStableCallback from "../../../hooks/__internal__/useStableCallback";
import useFormSpacing from "../../../hooks/__internal__/useFormSpacing";
import useInputAccessibility from "../../../hooks/__internal__/useInputAccessibility/useInputAccessibility";
import { OptionProps } from "../option";
import { OptionRowProps } from "../option-row";
import { CustomSelectChangeEvent } from "../simple-select";

let deprecateUncontrolledWarnTriggered = false;
Expand Down Expand Up @@ -345,6 +343,7 @@ export const MultiSelect = React.forwardRef(

const mapValuesToPills = useMemo(() => {
const canDelete = !disabled && !readOnly;
let matchingOptionValue: string;

if (!actualValue?.length) {
return "";
Expand All @@ -364,6 +363,11 @@ export const MultiSelect = React.forwardRef(

/* istanbul ignore else */
if (React.isValidElement(matchingOption)) {
matchingOptionValue =
matchingOption?.props.value?.value !== undefined
? matchingOption?.props.value?.value
: matchingOption?.props.value;

pillProps = {
title: matchingOption.props.text,
fill: matchingOption.props.fill,
Expand All @@ -373,10 +377,7 @@ export const MultiSelect = React.forwardRef(

const title = pillProps.title || /* istanbul ignore next */ "";
const key =
title +
((React.isValidElement(matchingOption) &&
(matchingOption.props as OptionProps | OptionRowProps).value) ||
/* istanbul ignore next */ index);
title + matchingOptionValue || /* istanbul ignore next */ index;

return (
<StyledSelectPillContainer key={key}>
Expand Down
21 changes: 21 additions & 0 deletions src/components/select/multi-select/multi-select.pw.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
MultiSelectErrorOnChangeNewValidation,
SelectionConfirmed,
MultiSelectWithDisabledOption,
OptionsWithSameName,
} from "../../../../src/components/select/multi-select/components.test-pw";
import {
commonDataElementInputPreview,
Expand Down Expand Up @@ -1130,6 +1131,26 @@ test.describe("MultiSelect component", () => {
await expect(selectInput(page)).toHaveCSS("border-radius", "4px");
await expect(selectListWrapper(page)).toHaveCSS("border-radius", "4px");
});

test("should delete options with when user clicks the close icon within a pill and there are options with same text", async ({
mount,
page,
}) => {
await mount(<OptionsWithSameName />);
await dropdownButton(page).click();

await selectOption(page, 2).click();
await selectOption(page, 0).click();
await selectOption(page, 1).click();

await expect(multiSelectPillByText(page, "Blue")).toHaveCount(1);
await expect(multiSelectPillByText(page, "Black")).toHaveCount(2);

await pillCloseIcon(page).nth(0).click();

await expect(multiSelectPillByText(page, "Blue")).toHaveCount(0);
await expect(multiSelectPillByText(page, "Black")).toHaveCount(2);
});
});

test.describe("Check events for MultiSelect component", () => {
Expand Down

0 comments on commit e09d172

Please sign in to comment.