support(Deeplink): Add Deeplink for Add Account and Accounts page #1247
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
π¦ Changeset detectedLatest commit: 9119933 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportBase: 48.12% // Head: 45.93% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1247 +/- ##
===========================================
- Coverage 48.12% 45.93% -2.19%
===========================================
Files 688 631 -57
Lines 30322 27517 -2805
Branches 7905 7372 -533
===========================================
- Hits 14591 12641 -1950
+ Misses 15668 14830 -838
+ Partials 63 46 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. β View full report at Codecov. |
Screenshots: β
It seems this PR contains screenshots that are different from the base branch.
windows
|
apps/ledger-live-mobile/src/components/RootNavigator/AddAccountsNavigator.tsx
Show resolved
Hide resolved
| @@ -5,7 +5,7 @@ import { filterPlatformApps } from "@ledgerhq/live-common/platform/filters"; | |||
| import { getPlatformVersion } from "@ledgerhq/live-common/lib/platform/version"; | |||
| import { NavigatorName, ScreenName } from "../const"; | |||
|
|
|||
There was a problem hiding this comment.
just a remark: I think that this whole file is useless and could be removed, in the end it's only used in apps/ledger-live-mobile/src/screens/NotificationCenter/NewsRow.tsx where the handler could be replaced by a simple call to
Linking.openURL (linking will then use the config specified in src/index.tsx to know how to handle the deeplink.
It would be a good opportunity to use a deeplinking related PR to do this clean up)
(There is just a need to double check that all the logic covered here is properly covered in the src/index.tsx config)
There was a problem hiding this comment.
actually im unsure it will work with Linking.openURL π€ thats why we made a seperate handler in the first place.
to try again later and remove it
There was a problem hiding this comment.
LGTM, I just highlighted in my previous comments some opportunities with this PR to tackle some tech debt:
- duplicated & out of sync documentation (wiki)
- possible (to double check) removal of
src/navigation/useDeepLinking.tswhich is just a duplication of the configuration ofLinkingalready implemented insrc/index.tsx - typing of some navigation routes -> actually I'm not sure this should be handled in this PR as there are already ongoing efforts to do that at the scope of the entire ap
mcayuelas-ledger
force-pushed
the
support/lllm-deeplinks-update
branch
from
8b41d7a
to
255644f
Compare
LFBarreto
force-pushed
the
support/lllm-deeplinks-update
branch
from
255644f
to
81f8d6e
Compare
π Description
Add Deeplink for Add Account and Accounts page
β Context
ledger-live-mobileβ Checklist
πΈ Demo
π Expectations to reach
Start LLM and use
adb shell am start -W -a android.intent.action.VIEW -d "ledgerlive://add-account?currency=ethereum" com.ledger.live.debugto trigger Deeplink for add-account. Ethereum should be filled by defaultPlease make sure you follow these Important Steps.
Pull Requests must pass the CI and be internally validated in order to be merged.