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

Tabbed Onboarding - Additional accounts #2956

Merged
merged 28 commits into from
Mar 10, 2023

Conversation

hyphenized
Copy link
Contributor

@hyphenized hyphenized commented Jan 31, 2023

Updates tabbed onboarding presentation when the user has at least one account in the wallet

Design:
Tabbed Onboarding
Additional accounts

Testing Env

SUPPORT_TABBED_ONBOARDING=true

To Test

  • Import a wallet, then try adding another account

Latest build: extension-builds-2956 (as of Fri, 10 Mar 2023 10:52:28 GMT).

@hyphenized hyphenized added this to the Onboarding Revamp milestone Jan 31, 2023
@hyphenized hyphenized self-assigned this Jan 31, 2023
@hyphenized hyphenized requested a review from a team January 31, 2023 16:29
@hyphenized hyphenized force-pushed the tabbed-onboarding-existing-accounts branch from 215cc50 to 1e31e14 Compare January 31, 2023 17:05
Copy link
Contributor

@jagodarybacka jagodarybacka left a 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:

ui/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/components/Onboarding/SupportedChains.tsx Show resolved Hide resolved
ui/components/Onboarding/SupportedChains.tsx Outdated Show resolved Hide resolved
ui/components/Onboarding/SupportedChains.tsx Show resolved Hide resolved
ui/pages/Onboarding/Tabbed/AddWallet.tsx Outdated Show resolved Hide resolved
ui/pages/Onboarding/Tabbed/Done.tsx Show resolved Hide resolved
ui/pages/Onboarding/Tabbed/SetPassword.tsx Outdated Show resolved Hide resolved
hyphenized and others added 4 commits February 4, 2023 16:29
Co-authored-by: Jagoda Berry Rybacka <[email protected]>
- Removed dead code
- Moved intersperse utility to ui/utils/lists
- Refactored styles
Copy link
Contributor

@kkosiorowska kkosiorowska left a 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.

Comment on lines +60 to +69
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"}
Copy link
Contributor

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"}

ui/pages/Onboarding/Tabbed/Done.tsx Outdated Show resolved Hide resolved
background/redux-slices/keyrings.ts Show resolved Hide resolved
Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

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

There are some design differences. Not only in this view. Should it be like this?

Screenshot 2023-02-13 at 11 53 3

Screenshot 2023-02-13 at 11 55 46

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
Copy link
Contributor

@kkosiorowska kkosiorowska left a 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.

  1. 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
  1. 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
alt="Tally Ho Gold"
alt="Taho Gold"

<img
width="80"
height="80"
alt="Tally Ho Gold"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
alt="Tally Ho Gold"
alt="Taho Gold"

@hyphenized
Copy link
Contributor Author

I took a quick look at it and it looks really good. ✨ I have two comments for this moment.

1. 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.

Nope, this is fixed in #2577 👀

Copy link
Contributor

@jagodarybacka jagodarybacka left a 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

ui/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/_locales/en/messages.json Show resolved Hide resolved
ui/package.json Outdated Show resolved Hide resolved
ui/pages/Onboarding/Tabbed/AddWallet.tsx Outdated Show resolved Hide resolved
ui/pages/Onboarding/Tabbed/Ledger/LedgerConnectPopup.tsx Outdated Show resolved Hide resolved
hyphenized and others added 2 commits March 9, 2023 12:42
- Move OnboardingAdditionalWallet to a new file
- Remove unused file
Co-authored-by: Jagoda Berry Rybacka <[email protected]>
Copy link
Contributor

@jagodarybacka jagodarybacka left a 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 🚀

#3139

@jagodarybacka jagodarybacka enabled auto-merge March 10, 2023 10:49
@jagodarybacka jagodarybacka merged commit 0162ce7 into main Mar 10, 2023
@jagodarybacka jagodarybacka deleted the tabbed-onboarding-existing-accounts branch March 10, 2023 10:52
@0xDaedalus 0xDaedalus mentioned this pull request Mar 30, 2023
kkosiorowska pushed a commit that referenced this pull request Apr 5, 2023
## 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants