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(@inquirer/core): Pass status to usePrefix/theme.message + Change default prefix #1544

Merged
merged 11 commits into from
Sep 15, 2024

Conversation

nvlang
Copy link
Contributor

@nvlang nvlang commented Sep 9, 2024

This PR includes 5 commits:

  • 82778c1 — Add .DS_Store to .gitignore for better DX on macOS
  • 6eb5381 — Pass status to usePrefix (instead of isLoading). Also updates references to isLoading in README. This allows for behavior like the one described in I want to change prefix after answered. #1537 to be implemented by the user.
  • eced5d6 — Pass status to style.message function as a second argument. This allows for behavior like the one described in I want to change prefix after answered. #1537 (comment) to be implemented by the user.
  • cc87d13 — Add TSDocs for Theme type (via local DefaultTheme type). This should improve IntelliSense.
  • bf0e8ea — Modify a unit test to test that the status property passed to usePrefix works as expected.

What's missing is a unit test for the changes to style.message. However, I couldn't immediately find unit tests for style.message to use as a template, and I didn't want to write one from scratch, since I'm not familiar with the setup. I tested the new style.message functionality locally via corepack yarn demo, and there it seemed to work fine.

Closes #1537.

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@d2690cc). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1544   +/-   ##
=======================================
  Coverage        ?   97.78%           
=======================================
  Files           ?       39           
  Lines           ?     2391           
  Branches        ?      644           
=======================================
  Hits            ?     2338           
  Misses          ?       46           
  Partials        ?        7           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@SBoudrias SBoudrias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on changing the default prefix?

* (text, status) => colors.bold(text)
* ```
*/
message: (text: string, status: string) => string;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give status specific allowed strings. (and everywhere relevant, there's quite a few so I won't call them all 😅 )

theme,
}: {
isLoading?: boolean;
status?: string;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give status specific allowed strings.

@nvlang
Copy link
Contributor Author

nvlang commented Sep 12, 2024

I think ? for when the question is pending and for when it's done could make sense? And maybe making the question mark blue and the checkmark green? I had left it alone since it seemed like it'd be a matter of personal preference, but I'd be glad to try to add whatever default you'd prefer.

Also, I wholeheartedly agree about giving status specific allowed strings — if I recall correctly, I had seen 'pending', 'loading', and 'done' being used, but also 'idle' in a unit test somewhere. I assume the first three should suffice, or would it make sense to include more statuses in your opinion?

@SBoudrias
Copy link
Owner

SBoudrias commented Sep 12, 2024

  1. Prefix, yeah blue/green question mark/check mark sounds good to me.

  2. I think we can go with 3 status. I prefer idle to pending; so I'd normalize this way. (maybe you'll find a valid 4th one when running typecheck, so low risk of missing one.)

The new default prefix is a blue question mark (`?`) if the status of
the prompt is not `'done'`, and a green heavy check mark (`✔`, U+2714)
if the status of the prompt is `'done'`.
@nvlang
Copy link
Contributor Author

nvlang commented Sep 13, 2024

@SBoudrias I guess changing the default prefix means that we should also update the screenshots in the READMEs — however, I'm not sure what the setup / workflow there is (theme, font, etc.).

@SBoudrias
Copy link
Owner

Thanks for the work you've put on that PR!

I went ahead a made a few adjustments before merging (just an FYI):

  • Used @inquirer/figures for the tick; since I don't think there was a fallback on windows.
  • Replaced the interface of prefix from a function to a record.
  • Updated documentation.

@SBoudrias SBoudrias merged commit 8bc4383 into SBoudrias:main Sep 15, 2024
9 of 10 checks passed
@SBoudrias SBoudrias changed the title Feat(@inquirer/core): Pass status to usePrefix and style.message (#1537) Feat(@inquirer/core): Pass status to usePrefix/theme.message + Change default prefix Sep 15, 2024
@SBoudrias
Copy link
Owner

SBoudrias commented Sep 15, 2024

About the screenshots, they were first introduced in #625... Feel free to regenerate screenshot and document a reproducible process in the CONTRIBUTING.md doc! (not sure if the same will work out of the box today 😬 - it's from 2018)

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.

I want to change prefix after answered.
2 participants