Skip to content

hopefully fix combobox key issue #2582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

hopefully fix combobox key issue #2582

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4705daf
hopefully fix combobox key issue
balupton Dec 28, 2019
f88b822
combobox: apply @dleroux suggestion
balupton Jan 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/components/Autocomplete/components/ComboBox/ComboBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,9 +459,7 @@ function assignOptionIds(
(
option: OptionDescriptor | ActionListItemDescriptor,
optionIndex: number,
) => {
option.id = `${comboBoxId}-${optionIndex}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was a little odd and might be obscuring what it's actually doing. Map returns a new array, however, we seem to be mutating the option and not using the result of the map. Ideally, we would have used a forEach, for, for of, etc.

},
) => ({id: `${comboBoxId}-${optionIndex}`, ...option}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we're not mutating the option anymore, or using the result of the map so it'll essentially a no-op.

Copy link
Author

@balupton balupton Jan 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndrewMusgrave would you be able to run with the appropriate amendments. If this is dependent on me, I am doubtful this is going to get fixed. No care if it is my PR that lands. Just wanting the issue fixed.

Copy link
Contributor

@alllx alllx Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use Array.map without use of newly returned array is an anti-pattern: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map#When_not_to_use_map
You can do this:

options.forEach(
    (
      option: OptionDescriptor | ActionListItemDescriptor,
      optionIndex: number,
    ) => {
      options[optionIndex] = {
        ...option,
        id: `${comboBoxId}-${optionIndex}`,
      };
    },
  );
  return options;

Also, in this fix I would not allow user to set his own id for option as you can see components still relies on his internal id naming in function private selectedOptionId().

);
return options;
}
Expand Down