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

Fix routing for creation of new wallet from action popup #4848

Closed
markmhendrickson opened this issue Jan 23, 2024 · 10 comments · Fixed by #4655
Closed

Fix routing for creation of new wallet from action popup #4848

markmhendrickson opened this issue Jan 23, 2024 · 10 comments · Fixed by #4655
Assignees
Labels
area:onboarding bug Functionality broken bug-p3 Non-critical functionality broken for many users, or there are clear workarounds

Comments

@markmhendrickson
Copy link
Collaborator

It currently goes to the full page view to the wrong route

routing.bug.mov
@markmhendrickson markmhendrickson added bug Functionality broken bug-p3 Non-critical functionality broken for many users, or there are clear workarounds area:onboarding labels Jan 23, 2024
@markmhendrickson markmhendrickson added this to the Fix urgent bugs milestone Jan 30, 2024
@pete-watters pete-watters self-assigned this Jan 30, 2024
@pete-watters pete-watters linked a pull request Jan 31, 2024 that will close this issue
21 tasks
@pete-watters
Copy link
Contributor

@markmhendrickson , just to confirm this.

  • As a user
  • WHEN I click 'Create new wallet' in the action popout
  • THEN the wallet SHOULD create a new wallet
  • AND land on the route /back-up-secret-key (below)

Image

@markmhendrickson
Copy link
Collaborator Author

Yep that's my expectation. cc @mica000 to confirm UX

@mica000
Copy link

mica000 commented Jan 31, 2024

yup!

@pete-watters
Copy link
Contributor

@kyranjamie I was going to change this but I just want to double check if there is a reason not to Create wallet in popup?

In WelcomePage we have some code that forces the user to go to Onboarding if they create wallet in the action popup.

I can change that but I'd like to make sure it doesn't introduce a regression.

 const startOnboarding = useCallback(async () => {
    if (isPopupMode()) {
      openIndexPageInNewTab(RouteUrls.Onboarding);
      closeWindow();
      return;
    }
    setIsGeneratingWallet(true);

@kyranjamie
Copy link
Collaborator

Yeah, was added intentionally. All it takes it one click to accidentally close the extension, then you have to start again (unless we manage a lot more onboarding state). Further, it's just generally a worse user experience to do it in a tiny mobile-sized viewport, when you could instead use a full page view.

@pete-watters
Copy link
Contributor

OK thanks Kyran. @markmhendrickson I won't change this as per comment above

@markmhendrickson
Copy link
Collaborator Author

markmhendrickson commented Feb 1, 2024

We still want to fix the routing though don't we? In that, the user indeed needs the full page view to open, but to the key generation screen, not getting started one.

@pete-watters
Copy link
Contributor

In the code we are doing a re-direct before creating anything.

I can change it but we will first have to create the new wallet in the extension and then do the re-direction.

const startOnboarding = useCallback(async () => {
    if (isPopupMode()) {
      openIndexPageInNewTab(RouteUrls.Onboarding);
      closeWindow();
      return;
    }
    setIsGeneratingWallet(true);
    // #4517 signout other tabs on wallet create
    await keyActions.signOut();
    keyActions.generateWalletKey();
    void analytics.track('generate_new_secret_key');
    if (decodedAuthRequest) {
      navigate(RouteUrls.SetPassword);
    }
    navigate(RouteUrls.BackUpSecretKey);
  }, [keyActions, analytics, decodedAuthRequest, navigate]);

I think this should be OK, but maybe @kyranjamie has better insights here.

To be clear, this is for creation of a new wallet, not recovery of a key.

@pete-watters
Copy link
Contributor

@kyranjamie - is it OK if I make this change so that:

  • when users click Create account in extension mode
  • we actually create the wallet for them and bring them here

You confirmed this was done intentionally here but Mark thinks we should still change it

@kyranjamie
Copy link
Collaborator

Mark and I are discussing complimentary, not contradictory, behaviours in these comments. I was saying that we should force the user to go to full screen mode. Mark is saying, yes do that, but take them directly to the save secret key page not the welcome page.

It is not a discussion about begin able to do onboarding entirely in the action popup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:onboarding bug Functionality broken bug-p3 Non-critical functionality broken for many users, or there are clear workarounds
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants