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

Resolve 547 #580

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Resolve 547 #580

wants to merge 5 commits into from

Conversation

jdlcdl
Copy link

@jdlcdl jdlcdl commented Jul 19, 2024

Description

resolves #547

Can now "Create a seed" from:

  • Seeds menu with seed(s) loaded instead of going thru "Load a seed", but title is "Create a Seed" and no Address Explorer/Verification offered

Flows supported are:

  • flow to it immediately in Seeds menu w/ seeds loaded... and that other tools are not available.
    SeedsMenuView ToolsViaCreateASeed

  • flow to it from "Load a seed" screen IF NO OTHERS ALREADY LOADED... and that other tools are not available.
    LoadSeedView
    TY Alvroble for the insight that to offer Create would be redundant (it was on the last screen) and confusing since Create and Load are being separated.
    LoadSeedView

  • flow to it from Tools menu, and the other tools ARE available.
    ToolsMenuView
    This is not new, but code changed in this pr does affect this view.

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before submitting the PR

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

  • No, I’m a fool
  • Yes
  • N/A

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.

@jdlcdl jdlcdl marked this pull request as draft July 19, 2024 21:09
@alvroble
Copy link
Contributor

As of e28308c 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:

  • "Tools" --> "New Seed"
  • "Seeds Menu" --> "Create a seed" --> "New Seed"
  • "Seeds Menu" --> "Load a seed" --> "Create a seed" --> "New Seed"

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

@jdlcdl
Copy link
Author

jdlcdl commented Jul 20, 2024

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 jdlcdl marked this pull request as ready for review July 29, 2024 17:52
@jdlcdl
Copy link
Author

jdlcdl commented Jul 29, 2024

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
Copy link
Contributor

easyuxd commented Aug 1, 2024

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
Copy link
Author

jdlcdl commented Aug 1, 2024

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
Copy link
Collaborator

tACK. LGTM

@newtonick newtonick added this to the 0.9.0 milestone Aug 30, 2024
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.

[Enhancement] Add Create Seed to In-Memory Seeds menu
4 participants