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

[LIVE-13346][LLM][BLE] Device search in BLE during onboarding doesn't filter on the selected device #7318

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

jiyuzhuang
Copy link
Contributor

@jiyuzhuang jiyuzhuang commented Jul 11, 2024

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • LLM onboarding flow has been updated;
    • Now the property filterByDeviceModelId of component BleDevicePairingFlow can be passed an array of DeviceModelId which will be transferred directly to filter (already supported). This can allow us show all devices with screen after scanning a QR code (no device info included for now).

📝 Description

  • Fix filter not work bug when clicking device.

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

@jiyuzhuang jiyuzhuang added the live-devices Device team label label Jul 11, 2024
@jiyuzhuang jiyuzhuang self-assigned this Jul 11, 2024
@jiyuzhuang jiyuzhuang requested a review from a team as a code owner July 11, 2024 13:33
Copy link

vercel bot commented Jul 11, 2024

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

5 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Jul 11, 2024 1:52pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Jul 11, 2024 1:52pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jul 11, 2024 1:52pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jul 11, 2024 1:52pm
web-tools ⬜️ Ignored (Inspect) Visit Preview Jul 11, 2024 1:52pm

@live-github-bot live-github-bot bot added the mobile Has changes in LLM label Jul 11, 2024
@@ -70,7 +70,7 @@ export const BleDevicePairingFlow: React.FC<Props> = ({ navigation }) => {
<Flex px={6} flex={1}>
<BleDevicePairingFlowComponent
key={keyToReset}
filterByDeviceModelId={DeviceModelId.stax}
filterByDeviceModelId={QRCodeDevices}
Copy link
Contributor

Choose a reason for hiding this comment

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

could be clearer with [DeviceModelId.stax, DeviceModelId.europa] 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.

It's a list that contains all the devices with screen and could be (surely) extensible in the future, so let's maintain the list to avoid modifying everywhere when a new device comes out.

Copy link
Contributor

Choose a reason for hiding this comment

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

This list could have a better name then, it's not clear to have QrCodeDevices filtered for BlePairingFlow imho

if (Array.isArray(filterByDeviceModelId)) {
return filterByDeviceModelId.filter(
// This array should not contain `null` value, this is to make the type check pass
(v: FilterByDeviceModelId): v is DeviceModelId => v !== null,
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, one char const/var should be avoided to understand more clearly :)

@gre
Copy link
Contributor

gre commented Jul 15, 2024

could you fill the PR description for Impact of the changes ?
the goal is just to make it explicit what does the code change impacts (for instance LLM device onboarding flow) in order for someone to be able to know exactly where to test 🙏

Copy link
Contributor

@gre gre left a comment

Choose a reason for hiding this comment

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

(some feedback in comment above, ✔️ overall)

@jiyuzhuang jiyuzhuang merged commit 1241c41 into develop Jul 15, 2024
39 of 41 checks passed
@jiyuzhuang jiyuzhuang deleted the bugfix/live-13346 branch July 15, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
live-devices Device team label mobile Has changes in LLM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants