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

✨feat(llm): add scan WS qr code #7512

Merged
merged 1 commit into from
Aug 8, 2024
Merged

✨feat(llm): add scan WS qr code #7512

merged 1 commit into from
Aug 8, 2024

Conversation

LucasWerey
Copy link
Contributor

@LucasWerey LucasWerey commented Aug 5, 2024

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • WS flow

📝 Description

Add the scan of LLD WS Qr code.
Change numbered list to fit with designs

Screen.Recording.2024-08-06.at.10.34.54.mov

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

@LucasWerey LucasWerey requested a review from a team as a code owner August 5, 2024 16:02
Copy link

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

@live-github-bot live-github-bot bot added mobile Has changes in LLM translations Translation files have been touched labels Aug 5, 2024
@live-github-bot
Copy link
Contributor

live-github-bot bot commented Aug 5, 2024

Mobile Bundle Checks

Comparing d39dde4 against 641cc17.

✅ Previous issues have all been fixed.

@live-github-bot live-github-bot bot added the ui Has changes in the design system library label Aug 6, 2024
@LucasWerey LucasWerey force-pushed the feat/LIVE-12168 branch 2 times, most recently from 53ec55b to ceb3409 Compare August 6, 2024 07:43
@live-github-bot live-github-bot bot removed the ui Has changes in the design system library label Aug 6, 2024
@LucasWerey LucasWerey force-pushed the feat/LIVE-12168 branch 3 times, most recently from bbf095b to ba9af29 Compare August 6, 2024 08:42
}: Props) => {
const { colors } = useTheme();

return (
<Flex flex={1} bg="background.main" px={6}>
<Flex flex={1} bg={hasNoBackground ? null : "background.main"} px={6}>
Copy link
Contributor

Choose a reason for hiding this comment

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

"transparent" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is it ?

You can it in package Icons ?

Copy link
Contributor Author

@LucasWerey LucasWerey Aug 6, 2024

Choose a reason for hiding this comment

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

That's the svg for the camera border not an Icon
It's already used for the original camera I've just duplicated it here and remove some parts of the svg

@@ -41,11 +40,12 @@ function View({
hasBackButton={canGoBack}
onBack={goBackToPreviousStep}
>
<Flex maxHeight={maxDrawerHeight}>
<Flex maxHeight={"90%"}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not 100%, even with 100% you still have a gap between top of screen if I'm correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not on some devices (with last iphones and the top bar it doesn't work)
image

@LucasWerey LucasWerey force-pushed the feat/LIVE-12168 branch 2 times, most recently from 2c87ba4 to 3380503 Compare August 6, 2024 09:14
@mcayuelas-ledger
Copy link
Contributor

Screenshot 2024-08-06 at 16 03 10 it should be like that

@LucasWerey
Copy link
Contributor Author

LucasWerey commented Aug 6, 2024

Screenshot 2024-08-06 at 16 03 10 it should be like that

@mcayuelas-ledger changed

Comment on lines 54 to 58
const onQrCodeScanned = (data: string) => {
// eslint-disable-next-line no-console
console.log(data);
// setCurrentStep(Steps.PinCodeInput);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in next PR :)

Base automatically changed from feat/LIVE-12167 to develop August 7, 2024 09:20
@LucasWerey LucasWerey dismissed mcayuelas-ledger’s stale review August 7, 2024 09:20

The base branch was changed.

@cgrellard-ledger
Copy link
Contributor

cgrellard-ledger commented Aug 7, 2024

Screenshot 2024-08-07 at 11 31 07

It looks weird with the corners inside of the black square instead of how it looks in the figma (at least we could put the same border radius on the black square and on the corners so it doesn't overflow)

Screenshot 2024-08-07 at 11 32 43

@LucasWerey
Copy link
Contributor Author

It looks weird with the corners inside of the black square instead of how it looks in the figma (at least we could put the same border radius on the black square and on the corners so it doesn't overflow)

Yes I know but in Android the camera can't be rounded so I need to see with Cédric what is the best solution

const { t } = useTranslation();

const QRSize = Math.round(width - 48);
const maxQRCodeSize = 280 - 15.36 * 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this 15.36 ?

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 has been done in previous PR I need to rebase

maxWidth={280}
maxHeight={280}
borderRadius={11.52}
background={"#fff"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the design system constant.white instead ?

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 has been done in previous PR I need to rebase

@LucasWerey
Copy link
Contributor Author

It looks weird with the corners inside of the black square instead of how it looks in the figma (at least we could put the same border radius on the black square and on the corners so it doesn't overflow)

Yes I know but in Android the camera can't be rounded so I need to see with Cédric what is the best solution

@cgrellard-ledger
image
I think it's better like that also it works for android that way

✨feat(llm): import account flow change

✨feat(llm): add back arrow to queued drawer

✨feat(llm): rework activation flow

✨feat(llm): clean

✨feat(llm): change walletsync to ledgersync for tracking

✨feat(llm): add qr code drawer from manage ws

✨feat(llm): refactor of ws setting inte test

✨feat(llm): rework activation flow

✨feat(llm): change walletsync to ledgersync for tracking

✨feat(llm): add scan WS qr code
@LucasWerey LucasWerey merged commit 7781e20 into develop Aug 8, 2024
35 of 43 checks passed
@LucasWerey LucasWerey deleted the feat/LIVE-12168 branch August 8, 2024 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants