pacocoursey / cmdk

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

Usually, IDs are case-sensitive, but cmdk transform them to lowercaes #150

Closed tea-artist closed 5 months ago

tea-artist commented 1 year ago

https://github.com/Fulament/cmdk/blob/86a1cba431e887a0dcf4788cc4a2dfc0cad0006f/cmdk/src/index.tsx#L155

useLayoutEffect(() => {
    if (value !== undefined) {
      const v = value.trim().toLowerCase()
      state.current.value = v
      schedule(6, scrollSelectedIntoView)
      store.emit()
Expand Down
Expand Up
    @@ -916,11 +916,11 @@ function useValue(
    const value = (() => {
      for (const part of deps) {
        if (typeof part === 'string') {
          return part.trim().toLowerCase()
        }

        if (typeof part === 'object' && 'current' in part && part.current) {
          return part.current.textContent?.trim().toLowerCase()
        }
      }
    })()

This makes me have to give up using the id returned in onChange.

I found a PR that attempted to address this issue. I believe it should be reopened and merged.

fevernova90 commented 1 year ago

Experiencing the same issue on v0.2.0.

Spent some time debugging why my id not matching the server, turned out that cmdk is lowercasing my ids.

Justinfan827 commented 1 year ago

Ran into this as well.

janhvipatil commented 1 year ago

same here, I had to add an extra function to get my original values after it lowercases them

Justinfan827 commented 1 year ago

My current approach is to use the value from the closure, but i'm not sure if this could cause issues with the variable being out of scope in the event handler?

Something like this:

<CommandGroup>
    {countryOptions.map((country) => {
      return (
        <CommandItem
          value={country.value}
          key={country.value}
          onSelect={(value) => {
            // Note: cmdk currently lowercases values in forms for some reason, so don't use
            // 'value' directly from the onSelect here
            form.setValue('country', country.value)
          }}
        >
          <Icons.check
            className={cn('mr-2 h-4 w-4', country.value === field.value ? 'opacity-100' : 'opacity-0')}
          />
          {country.label}
        </CommandItem>
      )
    })}
  </CommandGroup>
ashot commented 1 year ago

+1 the closure solution works, but will be brittle in case your code is not formatted inline

this should really solved by (optionally) separating the indexed string and the metadata returned from callbacks (id and value)

FerMPY commented 11 months ago

I also don't really get it, a better explanation would be good at least, but is definitely not useful most of the time you will almost always want value to be a id and the actual text you are selecting.

lawrencejob commented 10 months ago

Took me ages to find a) documentation of this feature and b) this thread (that helps me not feel insane after digging around for hours). It was doubly hard because I was using the shadcn/ui project, which has a subtle dependency on this one.

I'm guessing the lowercasing is to make indexing/searching more performant, but I think transforming the 'keys' is a leaky abstraction and there would be benefit in hiding the indexing behaviour from the user.

I happened to settle on the closure technique but I definitely don't feel great about it. Hopefully it's in a component I write once and never think about again. I happen to have >10,000 rows so I'm sure I'm pushing the component to an extreme in other ways already.

PS thanks for the great repo, it's made countless projects easier!

ahmadpak commented 8 months ago

My current approach is to use the value from the closure, but i'm not sure if this could cause issues with the variable being out of scope in the event handler?

Something like this:

<CommandGroup>
    {countryOptions.map((country) => {
      return (
        <CommandItem
          value={country.value}
          key={country.value}
          onSelect={(value) => {
            // Note: cmdk currently lowercases values in forms for some reason, so don't use
            // 'value' directly from the onSelect here
            form.setValue('country', country.value)
          }}
        >
          <Icons.check
            className={cn('mr-2 h-4 w-4', country.value === field.value ? 'opacity-100' : 'opacity-0')}
          />
          {country.label}
        </CommandItem>
      )
    })}
  </CommandGroup>

Thank you for the suggestion it helped. To avoid linting errors you can chose not to pass the value property to the onSelect function. image

pacocoursey commented 5 months ago

I agree this is wrong. Will remove.

WillSmithTE commented 4 months ago

wondering if it's possible to get a patch release with this change

adfdev commented 4 months ago

up

ht-lovrozagar commented 2 weeks ago

up