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: autocomplete multiselect #157

Merged
merged 7 commits into from
May 12, 2019

Conversation

creatyvtype
Copy link
Contributor

@creatyvtype creatyvtype commented Apr 16, 2019

Should handle #127
Adds figures 6 figures - arrows and radio on/off
Adds paginated scrolling for long lists in multiselect
Adds radio icons for multiselect (better than blank/check IMO)
Adds autocompleteMultiselect option

@lumio
Copy link
Collaborator

lumio commented Apr 17, 2019

Thanks for your PR.
What exactly was the problem with the local figures.js?

@creatyvtype
Copy link
Contributor Author

@lumio It was limited to only a handful of symbols, and I found this package added to include way more AND handle the Windows CMD fallbacks. I wanted the multiselect to have radio buttons, which were figures you did not have in the file, as well as using arrows to help provide instructions for the multiselect.

@terkelg
Copy link
Owner

terkelg commented Apr 20, 2019

I don't think we should introduce dependencies for this. If you need a few more signatures we can add them to figures.js

@creatyvtype
Copy link
Contributor Author

@terkelg this dependency is extremely small and essentially is exactly what you had for the figures. I feel like I would be literally copying from that library to include these icons here. Is it a blocker for you?

@lumio
Copy link
Collaborator

lumio commented Apr 29, 2019

I agree with @terkelg. Although it was meant in a good way (having the ability to use all supported characters of figures is a nice to have situation), adding a new dependency just for one or two newly used symbols sounds a little overkill to me.
One of prompts main goals is to have a minimal dependency footprint. There were a couple of incidents in the past that proved that this is actually a good thing to do.

Would it be ok with you to extend figures.js for the newly added symbols?

@creatyvtype
Copy link
Contributor Author

@lumio @terkelg I believe this should do it.

@terkelg
Copy link
Owner

terkelg commented May 7, 2019

Thank you. Does this look good to you @lumio?

@lumio
Copy link
Collaborator

lumio commented May 8, 2019

Sorry for the late response!
It looks really great. Only problem I see is that the autocomplete is case sensitive.

If I had an item Green and search for green there would be no match.
Other than that, it looks awesome

@creatyvtype
Copy link
Contributor Author

Also added check for title/value being a string since it doesn't seem the multiselect prompt checks for that.

@lumio
Copy link
Collaborator

lumio commented May 9, 2019 via email

@terkelg
Copy link
Owner

terkelg commented May 9, 2019

Thank you guys! Great feedback @lumio – appreciate it.
I'll check this out in the weekend and merge. High five!

@creatyvtype
Copy link
Contributor Author

Just so y'all know, I looked around at other prompt packages to find this feature and none of them had it. I chose to add it to yours because the code was VERY well organized and so easy to navigate compared to some of the others, so High Five right back at ya!

@lumio
Copy link
Collaborator

lumio commented May 10, 2019

Looks good to me ;) also the multiselect

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.

3 participants