-
Notifications
You must be signed in to change notification settings - Fork 394
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
Tabbed Onboarding - Additional accounts #2956
Conversation
- Add hook to determine if user is onboarding based on # of accounts
…ing locked If the user has already onboarded redirect to unlock signing screen before allowing to add new accounts
215cc50
to
1e31e14
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.
That's huge 😍 Some improvement ideas:
Co-authored-by: Jagoda Berry Rybacka <[email protected]>
- Removed dead code - Moved intersperse utility to ui/utils/lists - Refactored styles
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 haven't tested it yet but for now, it looks good. 😀. Some tiny comments.
src={ | ||
os === "mac" | ||
? `/images/mac-shortcut${altPressed ? "-option" : ""}${ | ||
tPressed ? "-t" : "" | ||
}.svg` | ||
: `/images/windows-shortcut${altPressed ? "-alt" : ""}${ | ||
tPressed ? "-t" : "" | ||
}.svg` | ||
} | ||
alt={os === "mac" ? "option + T" : "alt + T"} |
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 looks a little unclear. Maybe we should close it in the object? 🤔 Something like:
osMac = { name: "mac", altPressed: "-option", alt: "option + T"}
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 we're relying on the # of accounts to determine onboarding status, we have to persist onboarding status at the time the user starts the process, otherwise we don't show the right interface after finishing onboarding the first account into the wallet.
These shared styles were not correctly scoped with the previous approach
Display the correct screen when the user has not added accounts yet.
A recent chrome update broke these connection hints displayed during ledger onboarding
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 took a quick look at it and it looks really good. ✨ I have two comments for this moment.
- Is this the correct behavior for Rootstock Testnet? What I mean is that after selecting an option, I have to check the ledger again and again.
Screen.Recording.2023-03-07.at.15.39.53.mov
- I think it would be good to create some basic tests. There is a huge amount of work and change. Not in this PR but it would be useful to add them.
<img | ||
width="80" | ||
height="80" | ||
alt="Tally Ho Gold" |
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.
alt="Tally Ho Gold" | |
alt="Taho Gold" |
ui/pages/Onboarding/Tabbed/Done.tsx
Outdated
<img | ||
width="80" | ||
height="80" | ||
alt="Tally Ho Gold" |
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.
alt="Tally Ho Gold" | |
alt="Taho Gold" |
Co-authored-by: kkosiorowska <[email protected]>
Nope, this is fixed in #2577 👀 |
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 haven't tested it manually, doing it now. Overall it looks good, code comments below:
EDIT: UI looks 🔥 🔥 🔥 Manual testing approved. Will wait for your answers
- Move OnboardingAdditionalWallet to a new file - Remove unused file
Co-authored-by: Jagoda Berry Rybacka <[email protected]>
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.
All good, only the keyboard shortcut is not working but let's fix it later 🚀
## What's Changed * Fix network icon background url by @vvatom in #3136 * Fetch completed abilities by @kkosiorowska in #3072 * Tabbed Onboarding - Additional accounts by @hyphenized in #2956 * Update the spinner variant styles by @kkosiorowska in #3127 * v0.28.0 by @jagodarybacka in #3133 * [Custom RPCs] Allow displaying and removing custom networks by @hyphenized in #3032 * Fix CODEOWNERS teams and drop package.json by @Shadowfiend in #3143 * Onboarding - Remove supported chains, fix add wallet buttons by @hyphenized in #3159 * Delete accounts for a given network when removing that network. by @0xDaedalus in #3153 * Handle case where there are no cached assets for given network by @jagodarybacka in #3160 * E2E Tests - Intercept posthog requests in e2e tests by @hyphenized in #3146 * Token Balances - Prevent discovered tokens from shadowing known assets balances by @hyphenized in #3137 * Disable 429 Retry when querying CoinGecko API. by @0xDaedalus in #3165 * Does not show price impact when data is not ready by @kkosiorowska in #3157 * Add basic tests for abilities by @kkosiorowska in #3140 * Fix balance label width by @vvatom in #3134 * v0.28.1 by @hyphenized in #3167 * Unit tests for NFTItem by @kkosiorowska in #3173 * Onboarding Revamp QA by @hyphenized in #3097 * Impersonate MM on tally.xyz and polygon.technology by @jagodarybacka in #3194 * Replace deprecated `set-output` command by @michalinacienciala in #3199 * Custom RPCs - Update network icons color fallback list by @hyphenized in #3196 * NFT marketplace links revamp by @jagodarybacka in #3200 * E2E Tests - Add test for new onboarding flows by @hyphenized in #3113 ## New Contributors * @vvatom made their first contribution in #3136 * @michalinacienciala made their first contribution in #3199 **Full Changelog**: v0.28.1...v0.29.0 Latest build: [extension-builds-3210](https://github.com/tahowallet/extension/suites/11903950493/artifacts/623085934) (as of Thu, 30 Mar 2023 01:06:03 GMT).
Updates tabbed onboarding presentation when the user has at least one account in the wallet
Design:
Tabbed Onboarding
Additional accounts
Testing Env
To Test
Latest build: extension-builds-2956 (as of Fri, 10 Mar 2023 10:52:28 GMT).