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

Resolve 547 #580

Open jdlcdl opened 2 months ago

jdlcdl commented 2 months ago

Description

resolves #547

Can now "Create a seed" from:

Flows supported are:

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

As of https://github.com/SeedSigner/seedsigner/pull/580/commits/e28308cde026a63661ffbf75f729a3c28a75aabf ACK and tested.

Thank you for this enhancement. I think the "Create a seed" option is currently a bit hidden when seeds are already loaded, and this fixes that 👍.

From a functional point of view, it works as expected. However, one thing worth considering is removing the "Create a seed" option from LoadSeedView, as this button just appears on the previous screen. For consistency, I think it is better to have a clear path to each function in these situations, and having these three paths seems a bit confusing to me:

Maybe the first two paths would be sufficient. Of course, this is just my opinion.

jdlcdl commented 2 months ago

Maybe the first two paths would be sufficient. Of course, this is just my opinion.

I agree. I almost disabled this 3rd path but at last moment thought "What about users who already expect it here?"; at least for one release cycle until they get used to the more direct path.

I'll count your vote to remove it as 1, my own want to do the same as 1/2, and will wait to hear what others think. till then, I have more tests to get done and am currently testing the current release candidate.

Thank you for your review @alvroble!

jdlcdl commented 2 months ago

Ready for review, but STILL UNRESOLVED: Need opinion from other devs.

To remove "Create a Seed" from the "Load a Seed" menu when other seeds are already loaded? It doesn't need to be here any longer, it was in the previous screen just below "Load a Seed".

easyuxd commented 1 month ago

Agree with you both on removing "Create a seed" from the "Load a seed" menu, since these functions are now separated. Great catch, @alvroble and @jdlcdl!

Thanks for making this QoL enhancement a reality, @jdlcdl!

jdlcdl commented 1 month ago

ty @alvroble and @easyuxd for your thoughts on keeping "Create a seed" and "Load a seed" separated. This particular behavior is implemented as of c3ddd34.

newtonick commented 4 weeks ago

tACK. LGTM