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

Added pause prompt #108

Merged
merged 5 commits into from
Feb 21, 2024
Merged

Added pause prompt #108

merged 5 commits into from
Feb 21, 2024

Conversation

allanmcarvalho
Copy link
Contributor

This PR introduce the pause prompt that will be useful in a lot of scenarios. Eg.:

  • User need to read and accept a license. Pressing ENTER he agrees with license, or press CTRL-C to cancel (sample bellow).
  • The system need to know if users wants to continue with some action that can't be undone.
  • The system need to make a pause and show some content for user before continue.

Captura de Tela 2024-02-08 às 14 19 52

@taylorotwell taylorotwell marked this pull request as draft February 9, 2024 15:28
@jessarcher
Copy link
Member

Hey @allanmcarvalho, I'm in favour of a pause function, but I'm not sure about the UI.

I've had a similar use case previously, but I just wanted a simple "Press enter to continue..." to allow the user to read the previous output. In that scenario, I wouldn't want a box with text, as the output I want them to read had already been written.

What are your thoughts on something more simple API like:

function pause($message = 'Press enter to continue...'): void
{
    //...
}

Which would just output something similar to the info function:

image

In your case, you could output the license separately, followed by a pause. Although I'd probably use a confirm rather than pause to get an explicit yes/no.

@allanmcarvalho
Copy link
Contributor Author

allanmcarvalho commented Feb 19, 2024

Hi @jessarcher! Thanks by your review, you are definitely right. I will refactor with your suggestions ASAP and then push here. Have a good week!

@allanmcarvalho
Copy link
Contributor Author

allanmcarvalho commented Feb 19, 2024

@jessarcher I made the suggestions that you provided.

warning("Warning: The changes you are about to make cannot be undone. Please proceed with caution.");

pause();

info('Ok, processing...');

will output after press enter
Captura de Tela 2024-02-19 às 16 10 02

and then
Captura de Tela 2024-02-19 às 16 10 14

Message attribute is present, but optional and it's default is Press enter to continue....

What do you think now?

@jessarcher
Copy link
Member

Hey @allanmcarvalho,

Looks good! I changed it so that when the terminal is non-interactive, the pause has no effect instead of throwing a validation error. The return value will be false if the prompt wasn't rendered in case someone needs to know that. I don't think pause should be used to get a confirmation for destructive actions - just to pause the output to allow the user to read the previous output before continuing execution. It's almost like a confirm prompt where you can't select "No" (but can obviously Ctrl-C, potentially leaving things only halfway complete).

As mentioned previously, for scenarios where you need explicit confirmation from the user before doing something potentially destructive, I'd recommend using the confirm prompt (along with a custom --force option to handle non-interactive scenarios). This is what we do in the framework with the ConfirmableTrait (See MigrateCommand for example usage)

What are your thoughts?

@allanmcarvalho
Copy link
Contributor Author

Yes, I think that's it. Your point of view appears to well describe the objective behind of creation of this prompt. To be honest, I had the idea to create this prompt in a scenario where I needed to make a pause to read the previous content, so, it's like you described.

For scenarios where the user must to agree if continues or exit, maybe in future an exclusive prompt that made this like a charm can be crafted, but definitely it's not a job for pause.

Thanks for you review and contribution.

@allanmcarvalho allanmcarvalho marked this pull request as ready for review February 21, 2024 02:21
@taylorotwell taylorotwell merged commit 81b58bb into laravel:main Feb 21, 2024
4 checks passed
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.

3 participants