-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: show checkedName for selected checkbox options #1844
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
base: main
Are you sure you want to change the base?
feat: show checkedName for selected checkbox options #1844
Conversation
1713e40
to
02928ea
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1844 +/- ##
=======================================
Coverage ? 95.74%
=======================================
Files ? 45
Lines ? 2772
Branches ? 750
=======================================
Hits ? 2654
Misses ? 109
Partials ? 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hey @SBoudrias |
Hey @SBoudrias , I’m still running into a TypeScript issue with the checkedName property. After adding it internally in Choice, all local tests pass, but the e2e demo fails with:
I’ve checked the types exposed publicly in the checkbox package and I’m unsure what the preferred approach is. Should checkedName be added to the public Choice type in CheckboxConfig, exported separately, or handled another way? I’d appreciate your guidance so I can finalize the PR. Thanks! |
I believe it's an issue with the e2e test itself - we never added a new type this way, so the test didn't run into this case where it needs to whole package re-generated. |
Hey @SBoudrias , thanks a lot for looking into this and fixing the e2e issue — really appreciate it! |
.github/workflows/main.yml
Outdated
yarn set resolution @inquirer/prompts/@inquirer/checkbox@npm:* file:/tmp/artifacts/inquirer-checkbox.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/confirm@npm:* file:/tmp/artifacts/inquirer-confirm.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/editor@npm:* file:/tmp/artifacts/inquirer-editor.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/expand@npm:* file:/tmp/artifacts/inquirer-expand.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/input@npm:* file:/tmp/artifacts/inquirer-input.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/number@npm:* file:/tmp/artifacts/inquirer-number.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/password@npm:* file:/tmp/artifacts/inquirer-password.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/rawlist@npm:* file:/tmp/artifacts/inquirer-rawlist.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/search@npm:* file:/tmp/artifacts/inquirer-search.tgz | ||
yarn set resolution @inquirer/prompts/@inquirer/select@npm:* file:/tmp/artifacts/inquirer-select.tgz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what's breaking the e2e test.
Resources:
yarn set resolution <descriptor> <resolution>
The <descriptor>
isn't correct to link the prompt packages required by @inquirer/prompts
to use the one from the repo.
I'm short on time to figure the exact syntax, but that's the problem. Hopefully it gives you enough to research a bit the issue 🤞🏻
This PR updates the checkbox prompt to display the
checkedName
property for selected options,falling back to
name
ifcheckedName
is not provided. It includes:normalizeChoices
to handlecheckedName
.renderItem
logic to displaycheckedName
when an item is checked.This improves the clarity of selected options in prompts without breaking existing functionality.
Resolves #1647