-
-
Notifications
You must be signed in to change notification settings - Fork 285
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
Usually, IDs are case-sensitive, but cmdk transform them to lowercaes #150
Comments
Experiencing the same issue on v0.2.0. Spent some time debugging why my id not matching the server, turned out that |
Ran into this as well. |
same here, I had to add an extra function to get my original values after it lowercases them |
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> |
+1 this should really solved by (optionally) separating the indexed string and the metadata returned from callbacks (id and value) |
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. |
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 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! |
I agree this is wrong. Will remove. |
wondering if it's possible to get a patch release with this change |
up |
1 similar comment
up |
https://github.com/Fulament/cmdk/blob/86a1cba431e887a0dcf4788cc4a2dfc0cad0006f/cmdk/src/index.tsx#L155
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.
The text was updated successfully, but these errors were encountered: