Skip to content

Conversation

@dreyfus92
Copy link
Member

closes: #209

@changeset-bot
Copy link

changeset-bot bot commented Apr 7, 2025

🦋 Changeset detected

Latest commit: 5b2b75b

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

This PR includes changesets to release 2 packages
Name Type
@clack/prompts Minor
@clack/core Minor

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Apr 7, 2025

@example/basic@example/changesets

npm i https://pkg.pr.new/bombshell-dev/clack/@clack/core@278
npm i https://pkg.pr.new/bombshell-dev/clack/@clack/prompts@278

commit: 5b2b75b

@43081j
Copy link
Collaborator

43081j commented Apr 7, 2025

we should already have some spinner tests in index.test.ts FYI

we don't yet test exits, but we do test everything else. so you'd have to copy your process exit stuff across

as for the change itself, I wonder if we should have some i18n-like concept in here. basically a map of all constant strings so people can override them in general

I'll have a think and re-review soon

@dreyfus92
Copy link
Member Author

we should already have some spinner tests in index.test.ts FYI

we don't yet test exits, but we do test everything else. so you'd have to copy your process exit stuff across

as for the change itself, I wonder if we should have some i18n-like concept in here. basically a map of all constant strings so people can override them in general

sounds good, i will tinker with those suggestions. thanks james! 😁

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks great! My only request would be using the existing updateSettings() function unless there's a good argument for a separate API here?

Comment on lines 784 to 789
const promptsSettings = {
messages: {
cancel: 'Canceled',
error: 'Something went wrong',
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Could this use the existing global settings object? We should already expose an updateSettings() function

Comment on lines 831 to 832
? (errorMessage ?? promptsSettings.messages.error)
: (cancelMessage ?? promptsSettings.messages.cancel);
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like how this override works!

@natemoo-re
Copy link
Member

As for @43081j's suggestion of i18n support, I think that's a great idea! Maybe we can track that as a follow-up. I haven't really seen localized CLI tooling before but that seems pretty amazing.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks great! Please update the changeset and exports before merging 🙏

@43081j 43081j force-pushed the feat/improve-cancel-msgs branch from f5773a8 to 37f6046 Compare April 10, 2025 10:26
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Last things!

@dreyfus92 dreyfus92 merged commit 729bbb6 into main Apr 12, 2025
6 checks passed
@dreyfus92 dreyfus92 deleted the feat/improve-cancel-msgs branch April 12, 2025 19:48
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.

Improve spinner's cancel messages

4 participants