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(llm): show WS qr code / pincode #7467

Merged
merged 13 commits into from
Aug 7, 2024
Merged

✨feat(llm): show WS qr code / pincode #7467

merged 13 commits into from
Aug 7, 2024

Conversation

LucasWerey
Copy link
Contributor

@LucasWerey LucasWerey commented Jul 30, 2024 •
edited
Loading

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • WS Flow
    • RN UI Lib
    • PinCode Display / Input / error Screens

📝 Description

Add show qr code to WS flow
UI is not totally he same as the figma since we don't have all the components in the lib. This will be changed once the lib rework will be over
All the logic related to WS will be implemented in a next PR

It has been decided to use only 1 drawer for all the steps for UX reasons. So an adjustment has been done

AddAccount flow refactored in #7520
NumberedList with bulletPoints added in #7512

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-01.at.15.58.33.mp4

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

@vercel Vercel
Copy link

vercel bot commented Jul 30, 2024 •
edited
Loading

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 Aug 7, 2024 8:47am
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Aug 7, 2024 8:47am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Aug 7, 2024 8:47am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Aug 7, 2024 8:47am
web-tools ⬜️ Ignored (Inspect) Visit Preview Aug 7, 2024 8:47am

@live-github-bot live-github-bot bot added mobile Has changes in LLM translations Translation files have been touched labels Jul 30, 2024
@LucasWerey LucasWerey force-pushed the feat/LIVE-12167 branch 3 times, most recently from 6b0f4b3 to aec03a3 Compare July 31, 2024 09:42
@vercel Vercel
Copy link

vercel bot commented Jul 31, 2024

Deployment failed with the following error:

Too many requests - try again in 59 seconds (more than 120, code: "api-deployments-flood-pro").

@LucasWerey LucasWerey force-pushed the feat/LIVE-12167 branch 2 times, most recently from dfc6312 to 4734473 Compare July 31, 2024 09:47
@live-github-bot live-github-bot bot added the ui Has changes in the design system library label Jul 31, 2024
@LucasWerey LucasWerey force-pushed the feat/LIVE-12167 branch 5 times, most recently from 3f36aef to c87534b Compare August 1, 2024 15:15
mcayuelas-ledger
mcayuelas-ledger reviewed Aug 1, 2024
View reviewed changes
apps/ledger-live-mobile/src/newArch/features/WalletSync/screens/Activation/ActivationDrawer.tsx Show resolved Hide resolved
apps/ledger-live-mobile/src/newArch/features/WalletSync/screens/Activation/ActivationDrawer.tsx Outdated
<ActivationFlow
startingStep={currentStep}
onStepChange={handleStepChange}
onGoBack={callback => (goBackCallback = callback)}
Copy link
Contributor

Choose a reason for hiding this comment

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

you should try using maybe refs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think ref would make this more complex to read

Copy link
Member

Choose a reason for hiding this comment

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

I think that we have this issue because we manage the different drawers in the Activation Flow and should be at the top level. Is it possible to do it that way or do we have any limitation?

Copy link
Contributor Author

@LucasWerey LucasWerey Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's possible I did it that way so we could call the flow without being in a drawer, but that's probably never going to happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅

.../ledger-live-mobile/src/newArch/features/WalletSync/components/Activation/ActivationFlow.tsx Outdated Show resolved Hide resolved
@LucasWerey LucasWerey force-pushed the feat/LIVE-12167 branch 4 times, most recently from cf99e26 to c89f4e8 Compare August 2, 2024 14:05
@LucasWerey LucasWerey marked this pull request as ready for review August 2, 2024 14:09
@LucasWerey LucasWerey requested a review from a team as a code owner August 2, 2024 14:09
@LucasWerey LucasWerey changed the title ✨feat(llm): show WS qr code ✨feat(llm): show WS qr code / pincode Aug 2, 2024
mcayuelas-ledger
mcayuelas-ledger reviewed Aug 5, 2024
View reviewed changes
apps/ledger-live-mobile/.unimportedrc.json Outdated Show resolved Hide resolved
apps/ledger-live-mobile/src/newArch/features/WalletSync/screens/Manage/index.tsx Outdated Show resolved Hide resolved
apps/ledger-live-mobile/src/screens/Settings/General/WalletSyncRow.tsx Outdated
Comment on lines 30 to 32
navigation.navigate(NavigatorName.WalletSync, {
screen: ScreenName.WalletSyncActivationSettings,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove it from screen so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the activation Flow is inside a drawer. This screen is mandatory for when the user has already a trustchain

mcayuelas-ledger
mcayuelas-ledger reviewed Aug 5, 2024
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.

Can you add an integration test that opens QRCode drawer?

@LucasWerey LucasWerey force-pushed the feat/LIVE-12167 branch 3 times, most recently from e0b8c1b to f5f105d Compare August 5, 2024 13:44
@LucasWerey LucasWerey merged commit 3de93cc into develop Aug 7, 2024
43 of 45 checks passed
@LucasWerey LucasWerey deleted the feat/LIVE-12167 branch August 7, 2024 09:20
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

@gre gre gre left review comments

@mcayuelas-ledger mcayuelas-ledger mcayuelas-ledger approved these changes

Assignees

@LucasWerey LucasWerey

Labels
mobile Has changes in LLM translations Translation files have been touched ui Has changes in the design system library
Projects
None yet
Milestone
No milestone
Development

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