pacocoursey / cmdk

Fast, unstyled command menu React component.
https://cmdk.paco.me
MIT License
9.42k stars 265 forks source link

Bugs when rendering the `<Command.List>` in a `<Portal>` #95

Closed nicholaschiang closed 6 months ago

nicholaschiang commented 1 year ago

This isn't necessarily a bug—as the documentation never states this is supported—but much of CMDK's functionality breaks down when the <Command.List> is rendered in a Radix <Portal> (e.g. when building a combobox component) and thus is not a child of the <Command> HTML component, which causes all of the ref.current.querySelectorAll to fail:

// cmdk/src/index.tsx#L404
function getSelectedItem() {
  return ref.current.querySelector(`${ITEM_SELECTOR}[aria-selected="true"]`)
}
function getValidItems() {
  return Array.from(ref.current.querySelectorAll(VALID_ITEM_SELECTOR))
}

It would be nice to be able to pass in a listRef property that points to the <Command.List> component like so:

const listRef = useRef()
return (
  <Command listRef={listRef}>
    <Command.Input />
    <Portal.Root>
      <Command.List ref={listRef}>
        <Command.Item>a</Command.Item>
        <Command.Item>b</Command.Item>
        <Command.Item>c</Command.Item>
      </Command.List>
    </Portal.Root>
  </Command>
)

CMDK would then be able to use that listRef (if it exists) in place of its built-in ref whenever possible.

If this seems like a viable solution @raunofreiberg @pacocoursey, let me know and I can throw up a PR.

It may also be informative to have some examples of implementing a <Combobox> using this library. The README describes "⌘K is a command menu React component that can also be used as an accessible combobox." For example, how exactly did Rauno use the 2019 version for Vercel's combobox? Adding an example snippet or implementation to the docs will hopefully prevent a bunch of naive, junior developers such as myself from creating issues like this one. :)

joaom00 commented 1 year ago

Makes sense to use Command.List as a container instead of Command and I think there is no need to add a listRef prop, would use the context to get the Command.List ref

thure commented 9 months ago

+1, this is kind of a big deal if this project should interop with Radix. It depends on Radix’s Dialog which also portals, so I'd be surprised if this isn’t breaking in a lot of common use-cases.

Would also be glad to submit a PR, I may need to fork this anyway to fix this for our own work.

yudinvictor commented 5 months ago

@pacocoursey Is this problem supposed to be solved in 0.2.1? I still can't output Command.List inside Radix.Portal as in the example above

totodot commented 5 months ago

@pacocoursey im using radix popover + cmdk. Without Portal it works ok, but when CommandList is wrapped into Portal its breaks. List is rendered correctly but all keyboard actions not working. Can't change and select items using arrow up/down and enter.

totodot commented 5 months ago

@pacocoursey I think, that something breaks during minification your source file https://github.com/pacocoursey/cmdk/blob/v0.2.1/cmdk/src/index.tsx I tried Copy/Paste that code directly into a project and it works perfectly.

stammy commented 5 months ago

@pacocoursey im using radix popover + cmdk. Without Portal it works ok, but when CommandList is wrapped into Portal its breaks. List is rendered correctly but all keyboard actions not working. Can't change and select items using arrow up/down and enter.

Ran into the same

tino-brst commented 4 months ago

@pacocoursey im using radix popover + cmdk. Without Portal it works ok, but when CommandList is wrapped into Portal its breaks. List is rendered correctly but all keyboard actions not working. Can't change and select items using arrow up/down and enter.

Same here

skortchmark9 commented 3 months ago

Same - the "optionally pass a ref" suggestion from comment:1 seems like a pretty flexible solve to me.