Skip to content
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.

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

[combobox]: because combobox selected-icon logic uses in-memory comparison selectedItems are missing the checkbox #7055

Closed
1 of 2 tasks
scottdickerson opened this issue Oct 15, 2020 · 7 comments

Comments

@scottdickerson
Copy link
Contributor

  • carbon-components
  • carbon-components-react

Detailed description

I'd like to pass in the selectedItem as external State to the ComboBox,

const items = [{id: 1, 'text'1]];
const [selectedItem, setSelectedItem] = useState(items[0]);

<ComboBox
   onChange={changedItem=>  setSelectedItem(changedItem)}
   items={items}
   selectedItem={selectedItem}
  ...

however, the setSelectedItem returns a new object in memory as the selectedItem I guess, because it no longer matches the in-memory version from items
Because the combobox uses an in-memory object comparison to determine whether the current item in the list should show the checkbox or not, this fails to match and the checkbox fails to show, so you end up with this type of situation
image

It should use the same type of logic as the logic to determine isHighlighted:
isHighlighted: highlightedIndex === index || selectedItem && selectedItem.id === item.id || false,

This code works around the issue

setSelectedItem(localStateItems.find(item => isEqual(item, changedItem))); 
@dakahn
Copy link
Contributor

dakahn commented Oct 26, 2020

Interesting bug 👍 Thanks for reporting. Added to the backlog

@emyarod
Copy link
Member

emyarod commented Jun 2, 2021

if I'm not mistaken selectedItem does not get updated with the prop. would setting initialSelectedItem work in your use case?

const items = [ {id: 1, text: 1 }];
const [selectedItem, setSelectedItem] = useState(items[0]);

<ComboBox
   onChange={changedItem=>  setSelectedItem(changedItem)}
   items={items}
   initialSelectedItem={selectedItem}
  ...

@scottdickerson
Copy link
Contributor Author

scottdickerson commented Jun 3, 2021 via email

@emyarod
Copy link
Member

emyarod commented Jun 3, 2021

so if I understand correctly a combination of using initialSelectedItem at the app level and adding the selectedItem id check in the component itself should fit your use case, is that right?

would you be able to create a reduced test case in Code Sandbox for easier testing and debugging given that using initialSelectedItem alone seems to work for the code block posted in the original issue body?

@scottdickerson
Copy link
Contributor Author

scottdickerson commented Jun 4, 2021 via email

@emyarod
Copy link
Member

emyarod commented Jun 4, 2021

I can add an id check and expose a compareItems callback, but I just want to make sure I'm correctly testing for your use case since I think those changes may be unrelated to the original issue, given your original code snippet (the onChange handler doesn't seem to follow the expected function signature). would you be able to create a reduced test case in Code Sandbox for easier testing and debugging?

const [selectedItem, setSelectedItem] = useState(items[0]);
return (
  <ComboBox
    {...comboBoxProps}
    items={items}
    onChange={({ selectedItem: changedItem }) => setSelectedItem(changedItem)}
    selectedItem={selectedItem}
  />
);

@joshblack
Copy link
Contributor

Going to close this out due to inactivity, feel free to comment and I can re-open!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants