-
Notifications
You must be signed in to change notification settings - Fork 294
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
✨ LLM / LLD - Europa PostOnboarding #6646
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 5 Ignored Deployments
|
apps/ledger-live-desktop/src/renderer/components/PostOnboardingHub/logic/index.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-mobile/src/screens/PostOnboarding/PostOnboardingHub.tsx
Outdated
Show resolved
Hide resolved
...e-desktop/tests/specs/market/market.spec.ts-snapshots/market-page-thb-countervalue-linux.png
Outdated
Show resolved
Hide resolved
...live-desktop/src/renderer/screens/settings/sections/Experimental/PostOnboardingHubTester.tsx
Outdated
Show resolved
Hide resolved
… recover entry point
2dfc4bb
to
f1b8480
Compare
@cgrellard-ledger Checked on LLM and LLD, QA OK 👍 |
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.
Some questions but the rest LGTM
const initIsActionCompleted = useCallback(async () => { | ||
setIsActionCompleted(completed || !!(await getIsAlreadyCompleted?.({ protectId }))); | ||
}, [setIsActionCompleted, completed, getIsAlreadyCompleted, protectId]); | ||
|
||
useEffect(() => { | ||
initIsActionCompleted(); | ||
}, [initIsActionCompleted]); |
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.
Do you really want to re-run the init
if some dep changed ?
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 makes sense to recompute the isActionCompleted state if a prop changes for some reason
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 it just felt weird to call multiple times a function called init
apps/ledger-live-desktop/src/renderer/components/PostOnboardingHub/PostOnboardingActionRow.tsx
Show resolved
Hide resolved
@@ -29,7 +29,7 @@ export default function RecoverBanner({ children }: { children?: React.ReactNode | |||
const recoverServices = useFeature("protectServicesDesktop"); | |||
|
|||
const recoverBannerIsEnabled = recoverServices?.params?.bannerSubscriptionNotification; | |||
const protectID = recoverServices?.params?.protectId ?? ""; | |||
const protectID = recoverServices?.params?.protectId ?? "protect-prod"; |
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.
We should probably have a constant for protect-prod
as I see it repeated multiple times
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.
Agreed 👍 , maybe a subject for the recover team though
apps/ledger-live-mobile/src/components/PostOnboarding/PostOnboardingActionRow.tsx
Show resolved
Hide resolved
useEffect(() => { | ||
completeAction(PostOnboardingActionId.assetsTransfer); | ||
}, [completeAction]); |
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.
Same question as other useEffect do we want to call this only onMount or whenever completeAction changes too ?
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'll update this one to make sure we do not call this one again in case the completeAction changes
504fe50
✅ Checklist
npx changeset
was attached.📝 Description
Added europa support to post onboarding on both LLM and LLD
Updated post onboarding copy and analytics
Added an entry point to recover in the post onboarding
Updated the design of the Post onboarding banner on LLD
❓ Context
🧐 Checklist for the PR Reviewers