Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.3k
fix: handle skipped questions in when with display option#1853
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 ourterms of service andprivacy 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?
fix: handle skipped questions in when with display option#1853
Conversation
codecovbot commentedOct 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #1853 +/- ##======================================= Coverage ? 95.74% ======================================= Files ? 46 Lines ? 2888 Branches ? 773 ======================================= Hits ? 2765 Misses ? 114 Partials ? 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fe5866b to7f04251CompareSakthieswaran-tech commentedOct 21, 2025
Hey@SBoudrias , I’ve added more test cases and all checks are passing. Could you take a look when you get a chance and let me know if anything needs changes? |
SBoudrias left a comment
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.
The PR is a little big, so I didn't review all of it in depth yet. Here's a few early thoughts to streamline the code a little.
| confirm:(question:TypedQuestion)=>{ | ||
| constdefaultVal=question.default; | ||
| constanswerText=defaultVal===true ?'Yes' :defaultVal===false ?'No' :''; | ||
| constprefix='?'; |
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 must be taken from the theme
| constanswerText=defaultVal===true ?'Yes' :defaultVal===false ?'No' :''; | ||
| constprefix='?'; | ||
| constline=`${prefix}${question.message}${answerText}`; | ||
| returncolors.dim(line); |
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 should also be based on the theme.
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.
Updated to use the makeTheme() method for getting the prefix instead of hardcoding '?', and replaced dim with style.help
packages/inquirer/package.json Outdated
| "run-async":"^4.0.5", | ||
| "rxjs":"^7.8.2" | ||
| "rxjs":"^7.8.2", | ||
| "yoctocolors-cjs":"^2.1.3" |
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.
Let's match the version from other packages in the workspace.
README.md Outdated
| @@ -1 +1 @@ | |||
| packages/prompts/README.md | |||
| packages/prompts/README.md | |||
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.
git checkout main -- README.md to undo this change.
packages/inquirer/src/ui/prompt.ts Outdated
| return{ display, ask}; | ||
| } | ||
| returnBoolean(shouldRun); |
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.
Let's normalize the return types so it's always consistent.
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.
Normalized the function to return {display, ask}.
packages/inquirer/src/ui/prompt.ts Outdated
| if(ask||display){ | ||
| returnof({ question, ask}); | ||
| } | ||
| returnEMPTY; |
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.
It's a little weird to read the code where we handle the skip here, and the rendering in the next event. Let's combine that logic in a single place.
3ecbfcd to8e76e3cCompare8e76e3c to5b30e81CompareSakthieswaran-tech commentedOct 28, 2025
Rebased with latest main. |
Sakthieswaran-tech commentedNov 12, 2025
Hi@SBoudrias , I’ve made the requested changes and resolved the conflicts. Please take a look when you have some time and let me know if I need to make any further changes. Thanks! |
Sakthieswaran-tech commentedDec 3, 2025
Hi! Just checking in on this PR whenever you get a chance. Let me know if anything else is needed from my side. |
Uh oh!
There was an error while loading.Please reload this page.