-
-
Notifications
You must be signed in to change notification settings - Fork 170
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
New "Instant Onboarding" UX flow to create accounts #3773
Conversation
yes it does exist, it is similar to dcaccount, it is basically setting credentials and advanced options directly from the qr code data instead of asking a webserver, it is documented at https://github.com/deltachat/interface/blob/master/uri-schemes.md#dclogin- |
1ce110a
to
e0f187e
Compare
src/renderer/components/screens/WelcomeScreen/OnboardingScreen.tsx
Outdated
Show resolved
Hide resolved
7c71f6e
to
9d6f9f2
Compare
7304d9b
to
73fee77
Compare
const matches = qrWithUrl.url.match(URL_REGEX) | ||
if (matches && matches.length > 0) { | ||
const { origin } = new URL(matches[0]) | ||
return origin | ||
} |
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.
what is this logic for? is .domain
from core not what we want here?
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.
The core unfortunately doesn't give us the URL schema (https:// etc.) and thus not a valid openable link. I could just prepend the schema, but to really respect the given URL and not guess anything I've decided to get it from the original source
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 think https is safe to assume and require here
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.
Since this code is already working plus without any assumptions over how it might come around in the future (for example, how about local development setups without SSL?) I wouldn't change it further
41fa3ec
to
ece0a39
Compare
src/renderer/hooks/useProcessQr.ts
Outdated
throw new Error('QR code needs to be of kind "login"') | ||
} | ||
|
||
const numberOfAccounts = (await getConfiguredAccounts()).length |
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.
the actual question is if your current account is configured or not.
if it is not configured ask to login, if it is already configured talk about adding another account, so that user is not scared to loose their account.
Or is there some good logical reason for making it based on account count that I missed?
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.
Or is there some good logical reason for making it based on account count that I missed?
The method returns a list of all configured accounts, if it's length is 0
then it means there's no configured account as you say.
I'm not sure if it should only check the current account or if there's other configured accounts at all (as it currently is implemented and always was). The translation strings can be interpreted as a general note that whatever I have currently running will not be deleted, I think this counts even if I don't have that account currently selected?
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.
ok I guess we can do this in another pr/iteration. iOS also counts the accounts, I assume android does too.
only counting configured accounts is already an improvement, so now it's shown correctly for the first account.
I think this counts even if I don't have that account currently selected?
You are already in the flow of creating a new account so it telling you about other existing accounts is a bit weird. There are only the two cases: you scan from configured account and you scan from unconfigured account (welcome screen).
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.
DCLOGIN code does AEAP now when run from an configured account it changes your email credentials after showing a dialog that promises you that your existing account will stay as it is.
thoughts for future:
But thinking about that a bit we should probably show a custom dialog in this case, you scanned an account qr code to you want move your account to this new address or do you want to login to a new account. But allowing AEAP to do with these codes is probably nothing that we should suggest to users as long as AEAP is still considered experimental
Thank you for testing the |
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.
functionality-wise this lgtm, i tested quite a bit - very nice, what an improvement, thanks a lot 🤘
there may be issues and cornercases we did not encounter yet - however, esp. for that i would merge more sooner than later, so that we can do beta-versions and testing - and have smaller PRs to review then. "If something happens, it happens out there!"
code-level-wise - to me, it looks perfectly fine - also due to iterations and suggestions from react-experts - but well, i cannot really judge as i am too far away from react & co
what happens if you click the scan QR code here after having setting name and avatar??? will desktop remember this? I assume it will then show the "set profile name and image" again after the QR is scanned and account is created, this could cause confusion and frustration for an user that doesn't onboard in the default/main instance: "I already set name and avatar and now get asked again and I lost the configured data!?" |
yes, it is remembered, just tested again. we can achieve the same probably relatively easily on android/ios by creating an unconfigured context only as needed (so, reusing it when the InstantOnboardingActivity is reopened) EDIT: but i noticed that avatar-selection in general stops working, there seems to be sth lost on recent changes :) maybe related to deltachat/deltachat-core-rust#5509 ? (if it requires core, we can still merge this PR and file an blocking issue for that) - but the flow is perfectly fine, the broken avatar is preserved when switching instances :) |
I've made an issue here: #3797 |
also add a documentation comment to that function I think this improves code/naming clarity
yeah, first platform with instant onboarding! congrats! 🎉👯♀️👯♂️ |
Wireframes: https://whimsical.com/instant-onboarding-TrFnChnvwpRQqTSqgXoVPw
Related translation PR: deltachat/deltachat-android#3001
Todo
nine.testrun.org
as default)DC_QR_ACCOUNT
QR codeDC_QR_ASK_VERIFYCONTACT
orDC_QR_ASK_VERIFYGROUP
QR code using secure join afterwardsDC_QR_ASK_VERIFYCONTACT
orDC_QR_ASK_VERIFYGROUP
QR codeDC_QR_LOGIN
QR codeDC_QR_ACCOUNT
QR codeCheckbox
component ✨Video
instant-onboarding-2024-04-24_18.11.51.mp4
Screenshots