Skip to content
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

feat(SelectMenu): allow creating option despite search #1080

Merged
merged 12 commits into from
Dec 15, 2023

Conversation

ineshbose
Copy link
Member

@ineshbose ineshbose commented Dec 8, 2023

πŸ”— Linked issue

Resolves #1068, fixes #1073

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Say I want to create an option with the name "enhance"; the menu won't show it as a option as we do have a search result, though its a subset, not an exact.

image

This PR should allow creating such options.

image

Change made: allow creatable to be set to "always" -- this also allows us to not have to add an additional prop and additional maintenance.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

vercel bot commented Dec 8, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
ui βœ… Ready (Inspect) Visit Preview Dec 15, 2023 10:39am

Copy link
Member

@ineshbose Thanks for starting this out! This should solve: #1068

@ineshbose
Copy link
Member Author

Amazing! I didn't know that issue was listed.

One thing we may need to consider is that the create option in such cases should appear on top of the options, rather than below it, so I'm seeing if we can reuse the template for the create option to be placed as the first li element.

Copy link
Member

Indeed, you can move it first! What will be the name of the prop?

@ineshbose
Copy link
Member Author

It just modifies creatable prop to take in a string value "always"

@ineshbose
Copy link
Member Author

In cases where there are no results and we have the message showing up, do we want this:

image

or this:

image

I guess ideally we'd want to hide it?

Also noticed that GitHub has the option at bottom:

image

while something like Vue-Multiselect at top:

image

@ineshbose ineshbose marked this pull request as ready for review December 8, 2023 19:39
@ineshbose
Copy link
Member Author

OK so I've left the option at the bottom for now, but the template is reusable as a VNode too if we ever want to change/add in the future.

@bdebon
Copy link

bdebon commented Dec 11, 2023

Wonderful guys! So nice to see that it's coming so fast!

@benjamincanac
Copy link
Member

@ineshbose I've updated the docs but I think there is an issue with the logic:

CleanShot.2023-12-13.at.11.50.02.mp4

Also, could it be possible to solve this at the same time? #1073

@ineshbose
Copy link
Member Author

@ineshbose I've updated the docs but I think there is an issue with the logic:

CleanShot.2023-12-13.at.11.50.02.mp4

Also, could it be possible to solve this at the same time? #1073

Yeah it's a separate issue. I found that the options get duplicated as well in the existing implementation. Are you planning a release soon? I could look into it later today.

Copy link
Member

I haven't planed to make a release in the coming weeks no, maybe at the beginning of January so there's no rush 😊

@ineshbose
Copy link
Member Author

Sure - that fix should be included now. Still not sure about duplicate options - possibly another PR for it!

@benjamincanac
Copy link
Member

Thanks 😊 Just tested it out and there is indeed an issue with duplicates but also when creating a second one, it deselects everything else. Do you want me to take a look?

CleanShot.2023-12-14.at.10.50.09.mp4

@ineshbose
Copy link
Member Author

Yeah if you can - I can't figure it out. Could give it another try over the weekend, but if you crack it before me - great! πŸ˜„

@benjamincanac
Copy link
Member

benjamincanac commented Dec 15, 2023

I've updated your PR but starting from scratch. I added a new showCreateOptionWhen prop instead of altering the existing boolean. I didn't stick with the node render also. Let me know what you think!

The options were unselected on second create because the id was missing in the example https://github.com/nuxt/ui/pull/1080/files#diff-e48cbf82e89bfa293e6057d3afcf0716f5568c61e32ed85966cd0ca3b6d3dec5R26

@ineshbose
Copy link
Member Author

Sure thanks.

My personal preference would go for reusing/expanding existing props, else there's a bigger ladder of props to maintain I think; same for VNodes when we may want to reuse the template and provide yet another prop for placement of create button at top of list or bottom, for example.

This is fine too, happy to have the functionality implemented - as long as it's good to you @benjamincanac πŸ™‚

@benjamincanac
Copy link
Member

The idea behind this new prop it that it defaults from the app config so users can choose to apply this behaviour to all the creatable select menu in their app.

I would also agree for the VNodes if it was done like this everywhere else. I just favor code consistency across components 😊

@ineshbose
Copy link
Member Author

Good point. LGTM!

@benjamincanac benjamincanac merged commit 0fdc8f7 into dev Dec 15, 2023
2 checks passed
@benjamincanac benjamincanac deleted the feat/select-menu/always-create branch December 15, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants