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: confirmation prompts #687

Merged
merged 6 commits into from
Sep 30, 2024

Conversation

cartercanedy
Copy link
Contributor

@cartercanedy cartercanedy commented Sep 27, 2024

Add confirmation prompt to prevent accidental command execution

Type of Change

  • New feature
  • UI/UX improvement

Description

Having an accidental keypress result in a significant environment change sucks. Adding a prompt to confirm the execution of a command makes it less likely that a user will have a destructive command accidentally execute and have to decipher the command source to revert the changes (if they even are revertible).

Testing

locally, tested aborting and confirming command selections

Impact

should prevent accidental neovim config deletions, at least :)

Issues / other PRs related

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

@cartercanedy cartercanedy force-pushed the confirmation-prompt branch 4 times, most recently from 59f0448 to b21895d Compare September 28, 2024 01:17
@lj3954
Copy link
Contributor

lj3954 commented Sep 28, 2024

Confirmation prompt doesn't need its own focus variant, we can use the generic float variant with slight refactoring. lj3954@0a17f42.
The only thing missing is exiting on q or <C-c>, which could be implemented by implementing a 'can_exit' method for focus & float, or something of the like.

@cartercanedy
Copy link
Contributor Author

I think the focus specialization is fine there, it prevents us from needing to polute the FloatContent interface for other implementors

@cartercanedy cartercanedy marked this pull request as ready for review September 28, 2024 19:36
@cartercanedy cartercanedy changed the title Draft: Feat: confirmation prompts Feat: confirmation prompts Sep 28, 2024
@lj3954
Copy link
Contributor

lj3954 commented Sep 28, 2024

I think the focus specialization is fine there, it prevents us from needing to polute the FloatContent interface for other implementors

I don't think it's "polluting" the trait. All that needs to be changed is to return an enum rather than a boolean, out of the handle_key function. It's probably more clear that way, since the variants can explicitly state what action is occuring ('None', 'Close', etc) rather than having to look at comments in the trait to explain what each boolean value corresponds to.

Copy link
Collaborator

@adamperkowski adamperkowski left a comment

Choose a reason for hiding this comment

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

Works as expected, no issues.

NIT:
We could add some colors or other "aesthetic" things (like the one i mentioned below for example) to the popup though.

When selecting more than 9 entries, the list looks something like this:

1. Alacritty         
2. Android Debloater 
3. Bash Prompt       
4. Bottles           
5. DWM-Titus         
6. Docker            
7. Fastfetch         
8. Grub Theme        
9. Kitty             
10. Linutil Installer
11. Office Suite     
12. ZSH Prompt

We could make it check for the number of selected entries and add an extra space after the number for every 9/10 so it'd look like this:

9.  Kitty             
10. Linutil Installer

@cartercanedy
Copy link
Contributor Author

cartercanedy commented Sep 28, 2024

@lj3954 sorry for jumping to conclusions, I do like what you did with FloatEvent and making the handle_key_event api more fluent, but I do think that the confirmation float and other potential features could benefit from specialization of Float for concrete types instead of being limited to accessing a blind FloatContent trait object

@cartercanedy
Copy link
Contributor Author

how about

 1. ...
 ...
10. ...

@adamperkowski
Copy link
Collaborator

how about

 1. ...
 ...
10. ...

Looks alright.

Copy link
Contributor

@nnyyxxxx nnyyxxxx left a comment

Choose a reason for hiding this comment

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

love it

@ChrisTitusTech ChrisTitusTech merged commit 7cc38df into ChrisTitusTech:main Sep 30, 2024
1 check 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.

Confirmation prompts
5 participants