Skip to content

Conversation

KMJ-007
Copy link

@KMJ-007 KMJ-007 commented Aug 15, 2024

Added feature for create new option when no results are found

there is one rendering but for create new, which is caused by commandempty component, which needs to get fixed, so until than making this draft

Copy link

vercel bot commented Aug 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
shadcn-multi-select-component ❌ Failed (Inspect) Aug 15, 2024 7:50am

Copy link

@CruseCtrl CruseCtrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've got this working on one of my projects, using this PR as inspiration. I've left a few comments, but I'm happy to make those changes or raise my own PR if you prefer

* If true, allows creating new options that are not in the original list.
* Optional, defaults to false.
*/
creatable?: boolean;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need this creatable prop - we could make it creatable if an onCreate function is provided. Otherwise, the two props depend on each other, and it's possible to submit invalid values by e.g. setting creatable={true} and not providing onCreate, or providing onCreate without making it creatable

<CommandInput
placeholder="Search..."
onKeyDown={handleInputKeyDown}
/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CommandInput also has its own copy of the search input at the moment: https://github.com/pacocoursey/cmdk/blob/main/cmdk/src/index.tsx#L786

I think it would be better to use the controlled version of the input by passing in inputValue and setInputValue, as described here: https://github.com/pacocoursey/cmdk/tree/main?tab=readme-ov-file#input-cmdk-input

It would also be a good idea to rename them to search and setSearch, to match the naming from cmdk

}
};

const handleInputChange = (value: string) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't being used, and can be removed

<CommandEmpty>No results found.</CommandEmpty>
<CommandEmpty>
{creatable ? (
<CommandItem onSelect={handleCreateOption}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't be displayed, because the filter will automatically hide this CommandItem when it doesn't match the search input. I've ended up with the following code:

<CommandEmpty className={cn({ p0: !!onCreate })}>
  {onCreate ? (
    <CommandGroup forceMount>
      <CommandItem
        forceMount
        onSelect={() => {
          if (search) {
            onCreate(search);
            toggleOption(search);
            setSearch("");
          }
        }}
      >
        <Plus className="mr-2 h-4 w-4" />
        Create &quot;{search}&quot;
      </CommandItem>
    </CommandGroup>
  ) : (
    "No results found."
  )}
</CommandEmpty>

The most important parts are wrapping the CommandItem in a CommandGroup, and adding forceMount to both the item and the group

@KMJ-007
Copy link
Author

KMJ-007 commented Dec 18, 2024

Thanks! I've got this working on one of my projects, using this PR as inspiration. I've left a few comments, but I'm happy to make those changes or raise my own PR if you prefer

go ahead!

currently occupied by other projects, so will not get that much time to work on this PR,

will see your comments and suggestions on what i can improve and learn from it,

you can continue changes in this PR also or if you want to raise your own i don't have a problem with that either

@sersavan sersavan marked this pull request as ready for review December 18, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants