-
Notifications
You must be signed in to change notification settings - Fork 315
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
FAT-100: Adaptations on the Manager for FTS #1279
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: a431563 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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: 43.57% // Head: 43.63% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1279 +/- ##
===========================================
+ Coverage 43.57% 43.63% +0.05%
===========================================
Files 610 619 +9
Lines 25490 26280 +790
Branches 6982 7117 +135
===========================================
+ Hits 11107 11466 +359
- Misses 13255 14761 +1506
+ Partials 1128 53 -1075
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: ✅
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
ea557b9
to
3fdc645
Compare
3990a85
to
7340e9b
Compare
import { useNavigation } from "@react-navigation/native"; | ||
import { Text, Flex, Icons, BottomDrawer } from "@ledgerhq/native-ui"; | ||
import { Device } from "@ledgerhq/live-common/lib/hw/actions/types"; | ||
import { useBleDevicesScanning } from "@ledgerhq/live-common/lib/ble/hooks/useBleDevicesScanning"; |
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.
🚨 2 imports with lib
🏃
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.
Good catch! fixed
import React, { useCallback, useMemo } from "react"; | ||
import { Trans } from "react-i18next"; | ||
import { useDispatch } from "react-redux"; | ||
import { Device } from "@ledgerhq/live-common/lib/hw/actions/types"; |
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.
🚨🚨🚨
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.
Good catch! fixed
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.
Please do not use any
. 🙏
apps/cli/src/commands-index.ts
Outdated
@@ -102,4 +102,4 @@ export default { | |||
user, | |||
version, | |||
walletconnect | |||
}; | |||
} as any; |
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.
Please do not use any
.
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.
is this one really a problem? it's requiring a type annotation due to references in the lib. But it's the CLI, will we enforce this rule even in the CLI?
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.
Done
) | ||
); | ||
}, | ||
} as any; |
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.
Please do not use any
.
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.
Done
@@ -101,13 +103,16 @@ registerTransportModule({ | |||
id.startsWith("usb|") | |||
? Promise.resolve() // nothing to do | |||
: null, | |||
discovery: Observable.create(o => HIDTransport.listen(o)).pipe( | |||
discovery: new Observable<DescriptorEvent<any>>(o => |
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.
Please do not use any
.
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.
using unknown
as this type is not very revelant for the context here
} = this.props; | ||
const onHideMenu = () => setShowMenu(false); | ||
|
||
const onSelect = (result: any) => { |
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.
Please do not use any
.
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.
Using unknown
instead as the result type from a device action is very "unknown"
if (!interactive) return events; | ||
return events | ||
.pipe( | ||
rxScan((acc: any[], value) => { |
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.
Please do not use any
.
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.
Typed it correctly
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.
Except the ongoing request changes from other people, looks good to me!
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.
Thanks for making the changes 🙇
📝 Description
This PR rebuilds the device selection screen, with pointers to the new onboarding and new BLE pairing flows.
It uses custom icons for the manager tab according to the last seen device.
It adds a new button on the manager for FTS device to trigger the custom image flow.
It revamps the "not onboarded" device action error with new copy, illustration and a button directing to the right onboarding flow according to the device.
❓ Context
ledger-live-mobile
✅ Checklist
📸 Demo
demo-new-manager-fts.mov
🚀 Expectations to reach