SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
699 stars 161 forks source link

[Enhancement] Add exit dialog when entering passphrase #563

Closed alvroble closed 2 months ago

alvroble commented 2 months ago

Enhancement made:

Description

When entering a long passphrase, one can push accidentally the back button, so an exit dialog is added to prevent the accidentally loss of all the work to type in a long passphrase.

A new Warning Screen view is added:

SeedAddPassphraseExitDialogView

The actual dialog may be a bit too sober and could be modified to a more friendly "Are you sure you want to cancel?"

This pull request is categorized as a:

Checklist

If you modified or added functionality/workflow, did you add new unit tests?

I have tested this PR on the following platforms/os:

Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

alvroble commented 2 months ago

Checks passed 😀✅

jdlcdl commented 2 months ago

Since a release is around the corner (currently targeting Monday July 8th for finalization of all prs), please consider joining the SeedSigner Community group on telegram and/or the SeedSigner New Devs group to argue for this pr.

And thank you for contributing.

jdlcdl commented 2 months ago

As of 59bcfda:

ACK for the concept because what's the point of a bip39 passphrase if its not a strong one? and this is useful for that ACK on the code review ACK tested on rpi0 dev device ACK on one more new contributor to the next release...

But I still think improvements might be made on the title/text, AND I'd like to have input from other devs.

Thank you Alvroble!

alvroble commented 2 months ago

Thank you for the comments, @jdlcdl! I thought this would be a good first issue to learn about SeedSigner dev environment.

I'd love to join the Telegram groups and get more involved around here. How can I join?

jdlcdl commented 2 months ago

Duh, I didn't even include the links. :p

https://t.me/seedsigner_new_devs for new devs, most appropriate for dev discussions https://t.me/joinchat/GHNuc_nhNQjLPWsS for the main SeedSigner Community discussions.

alvroble commented 2 months ago

The new proposed screen is the following:

SeedAddPassphraseExitDialogView

jdlcdl commented 2 months ago

w/o altering my comments above,

I repeat my ACK tested at 1670bf6

easyuxd commented 2 months ago

Love this, @alvroble. Solid enhancement that will help reduce mistakes.

Looking at the other messaging screens, I’d recommend content like this to align with the rest of the UX, specifically the “Discard Seed” dialog:

discard-dialog

We use "discard" elsewhere, and "keep/discard passphrase" clarifies the specific actions and aligns with the rest of the content.

Thanks for your contribution!

alvroble commented 2 months ago

Thanks @easyuxd for the comment.

I agree to mantain the UX alignment. However, maybe a better text fo the "Continue editing" button would be "Edit passphrase" instead of "Keep passhprase", because when you push that button you are not specifically keeping the passphrase you were writing, you are just going back to continue editing it. What do you think?

SeedAddPassphraseExitDialogView

easyuxd commented 2 months ago

@alvroble Agreed, "Edit" makes more sense here. Good catch!

alvroble commented 2 months ago

@easyuxd Updated in the new commit. Thanks for the feedback!

kdmukai commented 2 months ago

Looking pretty good!

Minor note on the warning text: "Discard passphrase and go back" has two small issues:

So... I'm not sure what text should go in the body.

alvroble commented 2 months ago

Thanks @kdmukai! I understand, it's kind of tricky because of the context and limited space.

Maybe something like: "Your current passphrase entry will be erased. Proceed?"

easyuxd commented 2 months ago

Maybe something like: "Your current passphrase entry will be erased. Proceed?"

I like this language. I do think it's a good practice to repeat the action, at least in the H1 and the CTAs -- it should be clear to users what their action is without having to read the whole screen, with the body copy as added context.

alvroble commented 2 months ago

Currently working in the dict approach as @kdmukai suggested. Regarding the warning screen, the text I proposed fits in three lines, which doesn't look quite right. Since we are already asking in the title, I removed the "Proceed?" to make it better looking. @easyuxd what do you think?

SeedAddPassphraseExitDialogView_1 SeedAddPassphraseExitDialogView

jdlcdl commented 2 months ago

as of d7efbb3 ACK tested

@alvroble, great job on being responsive and flexible with others, and thank you.

alvroble commented 2 months ago

Just merged and tested after resolving some conflicts with the current dev branch

newtonick commented 2 months ago

tACK

closes #510