bombshell-dev / clack

Effortlessly build beautiful command-line apps
https://clack.cc
5.25k stars 88 forks source link

feat(@clack/prompts): multiselect maxItems option #151

Closed Mist3rBru closed 10 months ago

Mist3rBru commented 10 months ago

This PR this introduces maxItems option to multiselect prompt, using the same logic as select prompt. Now both use the same utility function limitOptions that handles this logic.

Demo

https://github.com/natemoo-re/clack/assets/100330057/6e8ebb80-3070-4028-aacb-68d206a99f99

changeset-bot[bot] commented 10 months ago

🦋 Changeset detected

Latest commit: 3c739f060281cecb81fcfea5646e343696e9f2da

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | -------------- | ----- | | @clack/prompts | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

cpreston321 commented 10 months ago

Hey @Mist3rBru! Everything looks good besides the the submitted value. The old way would show something like this @scope/a, @scope/b, @scope/c, @scope/z.

image

vs

image
Mist3rBru commented 10 months ago

@cpreston321 This is from main branch, I did not changed this (state) behavior.

cpreston321 commented 10 months ago

@Mist3rBru Screenshot 1 was from main branch and screenshoot 2 was from your branch.

try it out in both: pnpm dev

Mist3rBru commented 10 months ago

@cpreston321 the screenshots are testing different cases, the main:line#62 is concatenating the result with \n on patch step, I can change it, but I did't it.

cpreston321 commented 10 months ago

@Mist3rBru I thought it more of a symptom more then a test case. Since the maxItems was moved to a single function.

So I figured it was it was a side effect. I don't think it was intentional on your side.

Mist3rBru commented 10 months ago

@cpreston321 it is fine 😄. I gonna change this patch result to match multiselect result

cpreston321 commented 10 months ago

LGTM @Mist3rBru thanks for the PR!

🚀