Reach-ui: [Combobox] Add id or custom value to ComboboxOption

[CodingDive]

🚀 Feature request

Upon rendering a list of items inside a Combobox, I'd love to pass the item id to each option without rendering the id. This would make it easier to handle the onSelect as I wouldn't need to filter the list by the selected item string to retrieve the id.

Current Behavior

As far as I know, only text can be passed to the value of ComboboxOption which will be rendered to the DOM.

Desired Behavior

Pass item id or custom data along.

Suggested Solution

Add an extra id or data prop to the ComboboxOption and pass it along to the onSelect handler of the Combobox.

<Combobox onSelect={(value, data) => void alert(`You selected item with index ${data.index}!`}>
  <ComboboxInput
    aria-label="Cities"
    className="city-search-input"
    onChange={handleChange}
  />
  <ComboboxPopover className="shadow-popup">
    <ComboboxList aria-label="Cities">
      {results.slice(0, 10).map((result, index) => (
        <ComboboxOption
          key={index}
          data={{index}}
          value={`${result.city}, ${result.state}`}
        />
      ))}
    </ComboboxList>
  </ComboboxPopover>
</Combobox>

What do you think? If data is the second argument of onSelect, this wouldn't be a breaking change.

[enforce-template-use[bot]]

Thank you for opening this issue

[CodingDive]

@chancestrickland I'd be happy to send a PR for this

[chaance]

@CodingDive Considering this! I certainly see the value but I'd want to think through the implementation a bit. Happy to review a PR if you want to move ahead.

[CodingDive]

🙄

[dusty]

Do you see any problems in just adding an onClick handler to the ComboboxOption component and using that instead of the onSelect?

<Combobox>
  <ComboboxInput
    aria-label="Cities"
    className="city-search-input"
    onChange={handleChange}
  />
  <ComboboxPopover className="shadow-popup">
    <ComboboxList aria-label="Cities">
      {results.slice(0, 10).map((result, index) => (
        <ComboboxOption
          key={index}
          value={`${result.city}, ${result.state}`}
          onClick={() => void alert(`You selected item with index ${data.index}!`}
        />
      ))}
    </ComboboxList>
  </ComboboxPopover>
</Combobox>

[CodingDive]

@dusty haven't tried this one yet, thanks for sharing. I guess when onSelect and onClick are both present, one would need to write some extra code to coordinate between the two. Until my PR gets merged, this seems like a good solution indeed.

[dusty]

I went ahead and removed onSelect entirely and am just using onClick, seems to be working fine.

[CodingDive]

I'm worried that it won't work for some keyboard control but since reach-ui
is so good at handling focus management correctly, the onclick may always
be called

On Thu, Aug 13, 2020, 03:34 Dusty Doris [e-mail] wrote:

I went ahead and removed onSelect entirely and am just using onClick,
seems to be working fine.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/reach/reach-ui/issues/502#issuecomment-673197254, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AG4BSRZTPX4YEOWD5OFLARDSAM7JVANCNFSM4LD4UUAQ
.

[dusty]

Good call, seems like that is a problem.

[igloude]

Bump on this one. Even exposing the selected option's key or something would be a huge help.

[indiesquidge]

@igloude, keys only make sense in the context of the surrounding array when looping over data, and serve as a hint to React, but they don’t get passed to components. If we want the same value in the component, we'd have to pass it explicitly as a prop with a different name.

Is there a problem you're trying to solve for here that managing your own custom option data doesn't cover? I'd love to have a CodeSandbox showing the issues you're running into.