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.

Sign up for GitHub

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

Jump to bottom

feat: Add final steps (succes,warning, quickActions, emptyState UI) for add account v2 workflow #8929

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

feat: Add final steps (succes,warning, quickActions, emptyState UI) for add account v2 workflow #8929

wants to merge 1 commit into from
+1,026 −282

Conversation

themooneer
Copy link
Contributor

@themooneer themooneer commented Jan 16, 2025 •
edited
Loading

✅ Checklist

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

📝 Description

This PR will cover the following use cases:

  1. Show a success screen with one or more added account with a currency color based gradient.
    -> When user clicks on an account item , a redirection to account details page
    -> when user clicks on add funds to my accounts -> a first drawer suggest the account list to select -> a second drawer will appear with buy/receive cta once an account is selected.
Screen.Recording.2025-01-16.at.16.42.27.mov
  1. Show a warning screen one single 0$ balance existing account.
    -> Here when you click on Add funds to my accounts -> a drawer with buy/receive (no account list drawer in this case)
Screen.Recording.2025-01-16.at.16.44.06.mov
  1. Show a specific empty state screen for Hedera with a custom NoAssociatedAccount component (secured with a feature flag )
Screen.Recording.2025-01-16.at.16.45.35.mov
  1. Show a generic empty state when the account cannot be added. (this use case can happen when there is no funded accounts found on the scan process and the creation of the 0 $ balance account failed) an edge case that needs a specific backend simulation.

The following component should be seen:
image

👩🏼‍🎨 Resources

Figma for add account full process
Figma for additional screen
My test case testing

📣 📣 🚨 Details to keep in mind while reviewing/testing this PR

  • The close CTA for the current state of developpement will be fixed in this ticket
  • Animation are not finalised , pending animation keys from the design team -> will be done in this ticket
  • Analytics cleanup are not in the scope of this PR. There is a dedicated taks
  • Do not test this flow starting by Receive, Buy or Swap. An adjusting plug-in is expected in this task

❓ 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)

@themooneer themooneer requested review from a team as code owners January 16, 2025 15:57
@vercel Vercel
Copy link

vercel bot commented Jan 16, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 4:42pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2025 4:42pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2025 4:42pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2025 4:42pm

@live-github-bot live-github-bot bot added mobile Has changes in LLM translations Translation files have been touched labels Jan 16, 2025
@themooneer themooneer force-pushed the feat/LIVE-15607 branch 2 times, most recently from 4ba26ad to 638d67a Compare January 16, 2025 15:59
@live-github-bot
Copy link
Contributor

live-github-bot bot commented Jan 16, 2025 •
edited
Loading

Desktop Bundle Checks

Comparing dcdcaf2 against c0bff4d.

🚀 renderer bundle size decreased (38mb -> 37.8mb). Thanks ❤️

Mobile Bundle Checks

Comparing dcdcaf2 against 5ab951d.

🚀 main.ios.jsbundle bundle size decreased (62mb -> 61.8mb). Thanks ❤️
🚀 main.android.jsbundle bundle size decreased (62mb -> 61.8mb). Thanks ❤️

mcayuelas-ledger
mcayuelas-ledger requested changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@mcayuelas-ledger mcayuelas-ledger left a comment

Choose a reason for hiding this comment

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

Do we plan to have some Integration tests ? :)

apps/ledger-live-mobile/src/families/hedera/NoAssociatedAccounts.tsx
@@ -16,10 +19,12 @@ type Props = {
// "no associated accounts" text when adding/importing accounts
function NoAssociatedAccounts({ style }: Props) {
const { colors } = useTheme();
const c = colors.live;
const { t } = useTranslation();
const c = colors.primary.c100;
Copy link
Contributor

Choose a reason for hiding this comment

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

could you rename it more explicitly please? :)

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

apps/ledger-live-mobile/src/families/hedera/NoAssociatedAccounts.tsx
const fontSize = 13;
const onPress = useCallback(() => Linking.openURL(urls.hedera.supportArticleLink), []);
return (
return !llmNetworkBasedAddAccountFlow?.enabled ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion imho, llmNetworkBasedAddAccountFlow?.enabled ? newFeature: oldFeature is more readable

(Not mandatory)

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

...Arch/features/Accounts/screens/ScanDeviceAccounts/components/CanCreateAccountAlert/index.tsx
import { View } from "react-native-animatable";
import { useTheme } from "styled-components/native";
import Circle from "~/components/Circle";
import VerticalGradientBackground from "~/newArch/features/Accounts/components/VerticalGradientBackground";
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.

Oops ! VsCode things
✅ done

@themooneer
Copy link
Contributor Author

Do we plan to have some Integration tests ? :)

yes in the making for some components

KVNLS
KVNLS reviewed Jan 17, 2025
View reviewed changes
Copy link
Member

@KVNLS KVNLS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you add some integration tests to cover the User Story, please? It can be done in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KVNLS KVNLS KVNLS left review comments

@mcayuelas-ledger mcayuelas-ledger mcayuelas-ledger requested changes

@lambertkevin lambertkevin lambertkevin approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@themooneer themooneer

Labels
mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@themooneer @KVNLS @lambertkevin @mcayuelas-ledger