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

Introduce AddWalletButton and connect it to the AddWallet flow #418

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

johnny9
Copy link
Contributor

@johnny9 johnny9 commented Aug 24, 2024

These commits enable the "Add Wallet" button at the bottom of the Wallet Select menu on Desktop

@johnny9 johnny9 force-pushed the add-wallet-button branch from e7747b2 to 4b79351 Compare August 24, 2024 04:44
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

4b79351
the space between the icon and the text is different than from master

Master

Screenshot from 2024-08-24 11-06-36

PR

Screenshot from 2024-08-24 11-06-48


maybe for follow up:
At the AddWallet page, the Skip button in in the right top should get a condition to only have it when in the onboarding flow. Now in the app when you click add wallet button -> Add Wallet page has a skip button, which makes no sense. a cancel would be more appropriate

@hebasto
Copy link
Member

hebasto commented Sep 2, 2024

cc @GBKS

@GBKS
Copy link
Contributor

GBKS commented Sep 11, 2024

Nice work adding this functionality. I did a review of the full flow (MacOS) and added my notes in the visual below. Some of them will not be relevant to the focus of this PR and should probably be better handled in a separate one. Let me know how you want to handle that.

Bitcoin Core App PR 418 review 240911

And in written form:

1. Wallet selector dropdown
1.1 All font sizes should be 15px (not 13, 14, and 15)
1.2 Reduce spacing between "+" icon and label in the "+ Add Wallet" button

2. Add a wallet screen
2.1 In the transition to this screen, the new screen should slide in from below (not from the right)
2.2 Replace "Skip" button in the top-right, but a "Cancel" button in the top-left (keep "Skip" on this screen when it is shown in the onboarding sequence)

3. Choose a wallet name screen
3.1 Replace "Continue" button label with "Next"

4. Choose a password screen
All good

5. Your wallet has been created screen
5.1 Do we want to offer a "Back" button here? It is convenient for users, yet also creates a new wallet every time they go back and forth - is that a problem?

6. Back up your wallet screen
6.1 Same question about the "Back" button as above
6.2 "View file" button label should be white
6.3 Reduce spacing between buttons to 20px
6.4 When exiting this screen, it should slide down

7. Activity screen post wallet creation
7.1 The newly created wallet should now be active

@jarolrod jarolrod added enhancement New feature or request Wallet labels Sep 11, 2024
@rabbitholiness
Copy link

On macOS, the wallet name screen would not allow me to add spaces to the wallet name
Screenshot 2024-09-18 at 10 38 10

}

Component {
id: addWallet
id: addWalletFlow
Copy link
Member

Choose a reason for hiding this comment

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

This should be rebased over #427

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tACK 694025a

@rabbitholiness, this is only the UI link to the existent flow (CreateName.qml - not included in this PR), where there's a validator (dev's note: regexp qt class is deprecated and needs to be updated) which needs to be modified to include spaces as you said and I tested it too. Also to that flow we need to add functionality to the "Import wallet" and "View file" buttons, make the encryption not mandatory and perhaps as in Qt switch to the recently created wallet.

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tACK 694025a

@GBKS
Copy link
Contributor

GBKS commented Dec 5, 2024

Nice one. Looks like there's an issue with the web prototype in that the menu does not disappear when entering the create wallet flow. I'll make a note of that.

Something I noticed is that clicking the balance always toggle the menu to be visible. But when it is visible, and I click the balance again, the menu should disappear. Not having looked at the code at all, but knowing similar setups from building web apps, my assumption is that there is the click-outside-of-the-menu event handles is the reason for this. The mouse-down on the balance is counted as a click outside of the menu, which causes it to close. Then right after the click on the balance is triggered. Since the menu is now closed, it shows the menu again. So that mouse-down on the balance should not trigger the close (if that makes sense).

Another tweak (outside of this PR probably) is to show a "Cancel" button in the top-left of the first screen of the create wallet flow. That flow can be entered in two ways, with each one requiring different top bar options, so they make sense in context:

  • First-use experience: "Skip" button on the right
  • In-app wallet creation: "Cancel" button on the left

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants