-
Notifications
You must be signed in to change notification settings - Fork 40
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 Create Single-Sig wallet flow #403
Conversation
This allows components using the ContinueButton to apply their own color variations.
fb61899
to
4c047e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- every entrybox should automatically be selected, so user doesn't have to move the cursor there and click it to enter something
- at Create password, good to give feedback when user can't continue because passwords don't match
CreateConfirm
dialog shouldn't have a back button imo, the wallet is already created so going back doesn't make sense, same forCreateBackup
some other things, but maybe for a follow-up:
- check and don't allow to enter an already existing wallet name
- at every entry it is intuitive to press enter to trigger the continue button, maybe good time to introduce the behavior that enter keystroke triggers it
- the transition to the wallet home page after clicking done at CreateConfirm is in the opposite direction of the rest of the create wallet flow, why?
CoreText { | ||
Layout.topMargin: 25 | ||
Layout.alignment: Qt.AlignCenter | ||
text: qsTr("You are about to create \na single-key bitcoin wallet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text: qsTr("You are about to create \na single-key bitcoin wallet") | |
text: qsTr("You are about to create \na single-sig bitcoin wallet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It says "single-key" also in figma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, "single-key" is wrong and also confusing imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur "single-sig" is more intuitive than "single-key". It seems "single-key" is referenced in the docs although most people/users only refer to "single-sig"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally use multi-key in the design community (example). Some reasons:
- "sig" is an abbreviation that many people may not be familiar with
- Avoiding "signature" altogether in the applications removes another technical concept that people don't really need to be aware of. For example, "sign transaction" can be just be the more common term "approve transaction". Just part of the overall idea to use fewer technical terms if possible
- If we want to be super specific in technical terms, then a "single-sig" wallet could even be a "multi-key" wallet. For example, in a wallet with 3 keys that only requires 1 signature
I understand that the technical terms can be more intuitive for us who are super deep into the tech, and they allow us to discuss things more precisely. For broader audiences, especially global ones where english may be the second language, it's often best to simplify things a little (you even have tools for that like Hemingway). Does that sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 4c047e0
It creates the wallet successfully, with or without password (skip it when it's offered).
I agree with all observations from @MarnixCroes as, tested on desktop platform, I experienced the same issues.
some other things, but maybe for a follow-up:
* check and don't allow to enter an already existing wallet name
This is what I first tested, when user enters the same wallet name doesn't do anything/ warn about the situation (doesn't override existent ones at least 🙏🏼) and it goes to the CreateConfirm
saying it was created.
* the transition to the wallet home page after clicking done at CreateConfirm is in the opposite direction of the rest of the create wallet flow, why?
I'd say opposite direction to the rest of the onboarding flow even, so create wallet flow follows the same pattern (screen passed to the left as we move forward to the next form/ screen to the right) until the user clicks on "Done" or even if the user "skip"s the wallet creation. Unless it's intentional, it seems there's an inconsistency here.
On CreateBackup.qml
, there's a "View file" button (that's doesn't work yet?), is this the backup feature? If so I see in figma says "Save wallet file" on that button, otherwise other than "CreateBackup" I'd name the .qml
file as "BackupIntro" o "BackupWarning"/ Info.
I see other discrepancies (missing options/ different wording) against figma, is this intentional?
CoreText { | ||
Layout.topMargin: 25 | ||
Layout.alignment: Qt.AlignCenter | ||
text: qsTr("You are about to create \na single-key bitcoin wallet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It says "single-key" also in figma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 4c047e0 on WSL Ubuntu 22.04.
Wallet creation worked. In general agree with Marnix's comments.
And to be clear is this the first iteration, then layering more functionality (i.e. error messages, wallet name collision and more specific passwords hints )?
Yeah this is just following what was ready in the Figma. I think building it helps identify all of the gaps. I think all of these comments are great and I'll do my best to get these in on this first change-set. |
qInstallMessageHandler(DebugMessageHandler); | ||
//qInstallMessageHandler(DebugMessageHandler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that commented out?
cc @GBKS |
Testing the PR, so far worked well and created a wallet. "View file" button didn't work at all though.
@johnny9 mentioned in the issue in design repo, the lastest Figma design is here and was not yet merged to main Figma file. It does not differ greatly from what's in this PR. Minor changes were introduced from user testing the flow. All differences should be highlighterd in the file and issue |
4c047e0
to
a3de255
Compare
tACK a3de255 on WSL Ubuntu 22.04 Works as expected, was able to create a new wallet in the default |
…lly selected 0beac80 qml: create wallet: set focus on TextField (Marnix) Pull request description: add `focus: true` to make it automatically selected so user doesn't have to first manually select it before it can type something addresses #403 (review) ACKs for top commit: johnny9: tACK 0beac80 Tree-SHA512: 61430831010dbd582176e9fd57ec7dcafbc5c705ee7ede7d7d33bc5d49e76340d4d233de385f2b59fc8c27d4422d3d82c30238678a5ee1429f7659800e6820b0
456d28a qml: uncomment DebugMessageHandler install (johnny9) Pull request description: DebugMessageHandler is needed to route output to debug.log and should not have been commented out. Mistakenly commented out in #403 ACKs for top commit: MarnixCroes: ACK 456d28a hebasto: ACK 456d28a Tree-SHA512: c78e526deccba366854287c796e2ac4843f1aba7b936d6955b6ec2212e75fac6a4372e3994e4045a45e083c7422fb827fddb674c64a593affa1226589f14d8ab
These changes add more pages after the initial onboarding.
To test, run the application with the
-resetguisettings
option.Link to github actions build artifacts.