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

feat: Scan & device accounts - add account v2 #8802

Merged
merged 6 commits into from
Jan 15, 2025
Merged

Conversation

themooneer
Copy link
Contributor

@themooneer themooneer commented Jan 3, 2025

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

πŸ“ Description

In the current feature:

  • After asset selection (crypto + network) , user will be redirected to device selection (with autoSkip and select the current connecting device same as Receive flow).
  • Once device is wired -> A scan of accounts process starts.
  • Accounts are displayed 1 by 1 (with a first version of animation).
  • User can stop the scan process if not finished.
  • User can select one-to-many accounts via checkbox / select all
  • User can rename an account (same ui of Add account v1)
  • Once selection is done a redirection to a UNCOMPLETE SUCCESS SCREEN (will be done LIVE-13983)
  • For this stage only funded accounts can be added , no warning screen/ui (LIVE-13983 scope)

πŸ“Ή Videos

selectAccTest1.mov

Notes ‼️

I will add more video related to funded importable accounts which i don't possess on my seed actually.
This won't prevent the PR from being reviewed or even tested if you have a device with multiple accounts for a single currency.

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

Copy link

vercel bot commented Jan 3, 2025

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

4 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Jan 15, 2025 3:32pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jan 15, 2025 3:32pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jan 15, 2025 3:32pm
web-tools ⬜️ Ignored (Inspect) Visit Preview Jan 15, 2025 3:32pm

@live-github-bot live-github-bot bot added mobile Has changes in LLM translations Translation files have been touched labels Jan 3, 2025
@themooneer themooneer force-pushed the feat/LIVE-15524 branch 2 times, most recently from 1bd5924 to 7788db9 Compare January 3, 2025 17:14
@live-github-bot
Copy link
Contributor

live-github-bot bot commented Jan 3, 2025

Mobile Bundle Checks

Comparing b2385a6 against 04af7d7.

βœ… Previous issues have all been fixed.

@themooneer themooneer force-pushed the feat/LIVE-15524 branch 11 times, most recently from 1ea3840 to 9dde117 Compare January 8, 2025 14:08
@themooneer themooneer marked this pull request as ready for review January 8, 2025 14:09
@themooneer themooneer requested a review from a team as a code owner January 8, 2025 14:09
@@ -28,6 +28,10 @@ import TouchHintCircle from "./TouchHintCircle";
import Touchable from "./Touchable";
import { AccountSettingsNavigatorParamList } from "./RootNavigator/types/AccountSettingsNavigator";
import { AddAccountsNavigatorParamList } from "./RootNavigator/types/AddAccountsNavigator";
import AccountItem from "~/newArch/features/Accounts/components/AccountsListView/components/AccountItem";
Copy link
Contributor

Choose a reason for hiding this comment

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

LLM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done βœ…

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have it in DS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to use the same icon for the LLD add account can we add it to the DS please ? I don't know if this doc is up to date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do it in a separate ticket once needed by LLD.

"scanningTitle": "Looking for any existing accounts on the Blockchain...",
"sections": {
"importable": {
"title": "We found {{count}} account"
Copy link
Contributor

Choose a reason for hiding this comment

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

it needs plural

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done βœ…

@@ -4002,6 +4002,8 @@
"desc": "Are you sure you want to cancel adding accounts?"
},
"imported": "Asset added successfully",
"added": "Account added to your portfolio",
"added_plural": "{{count}} Accounts added to your portfolio",
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
"added_plural": "{{count}} Accounts added to your portfolio",
"added_plural": "{{count}} accounts added to your portfolio",

I'm not sure but should be lowercase imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
it's for the success screen , in figma it's uppercase

"title": "Accounts already in the Portfolio ({{length}})"
},
"migrate": {
"title": "Accounts to update"
Copy link
Contributor

Choose a reason for hiding this comment

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

should be singular also no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no idea about this one, as i'm using the exact same wording of add account v1 , see the same object above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as there is no count there , i prefer leave it like add account v1 wording

Comment on lines 8 to 9
import Button from "~/components/Button";
import LText from "~/components/LText";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here better to use DS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good to know, i'll do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done βœ…

Comment on lines 11 to 12
import ExternalLink from "~/icons/ExternalLink";
import Info from "~/icons/Info";
Copy link
Contributor

Choose a reason for hiding this comment

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

those also, if they are available :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good to know ! (well while i'm using the same as this component apps/ledger-live-mobile/src/screens/AddAccounts/03-Accounts.tsx , i didn't knew that ) thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done βœ…


import { accountsSelector } from "~/reducers/accounts";
import { blacklistedTokenIdsSelector } from "~/reducers/settings";
import { Theme, withTheme } from "../../../../../colors";
Copy link
Contributor

Choose a reason for hiding this comment

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

from "styled-components/native"; ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

import Button from "~/components/Button";
import PreventNativeBack from "~/components/PreventNativeBack";
import Chevron from "~/icons/Chevron";
import LText from "~/components/LText";
Copy link
Contributor

Choose a reason for hiding this comment

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

can you replace this by as you use it already? :)

I know it's not the scope but could be good !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done βœ…

@@ -43,7 +43,7 @@ export default function SelectNetwork({
onPress={onPressItem}
subTitle={
item.accounts.length > 0
? t("transfer.receive.selectNetwork.account", { count: item.accounts.length })
Copy link
Contributor

Choose a reason for hiding this comment

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

if not used anymore could you remove it from common.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's used in the receive flow

@themooneer themooneer force-pushed the feat/LIVE-15524 branch 3 times, most recently from 2376268 to c83b9ac Compare January 8, 2025 15:48
inline?: boolean;
returnToSwap?: boolean;
onSuccess?: (res: { scannedAccounts: Account[]; selected: Account[] }) => void;
};
[ScreenName.SelectAccounts]: CommonParams & {
currency: CryptoCurrency | TokenCurrency;
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
currency: CryptoCurrency | TokenCurrency;
currency: CryptoOrTokenCurrency;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done βœ…

import { ScreenName } from "~/const";
import { Device } from "@ledgerhq/types-devices";
import { AccountLikeEnhanced } from "../ScanDeviceAccounts/types";
import { Account } from "@ledgerhq/types-live";
import { CryptoCurrency, TokenCurrency } from "@ledgerhq/types-cryptoassets";
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
import { CryptoCurrency, TokenCurrency } from "@ledgerhq/types-cryptoassets";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done βœ…

});
}, [account, navigation, onAccountNameChange]);
if (llmNetworkBasedAddAccountFlow?.enabled) {
(navigation as StackNavigationProp<AccountSettingsNavigatorParamList>).navigate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this type assertion here ? Is there a way to avoid it ? We should avoid using type assertions as much as possible as they can introduce nasty and hard to debug errors

padding="8px"
columnGap={8}
>
<AccountItem account={account as Account} balance={account.spendableBalance} />
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done βœ…

@themooneer themooneer merged commit b447baf into develop Jan 15, 2025
40 of 41 checks passed
@themooneer themooneer deleted the feat/LIVE-15524 branch January 15, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants