Skip to content

add progress of scanning QR code#20947

Merged
Akaryatrh merged 3 commits intoMetaMask:developfrom
KeystoneHQ:add-progress
Jan 31, 2024
Merged

add progress of scanning QR code#20947
Akaryatrh merged 3 commits intoMetaMask:developfrom
KeystoneHQ:add-progress

Conversation

@renfengshi
Copy link
Copy Markdown
Contributor

@renfengshi renfengshi commented Sep 19, 2023 •
edited by plasmacorral
Loading

Description

When using with a QR code hardware wallet, if the data is large, it will take a lot of time, but users cannot get any response during the scan. Adding a scanning progress will give users a better experience.

Manual testing steps

Edited 2/21/2024 after the fact based on new information

  1. Unlock Keystone
  2. Access 3 dots menu
  3. Tap Connect Software Wallet
  4. Tap MetaMask
  5. Tap 3 dots menu from Connect MetaMask
  6. Tap Change Derivation Path
  7. Tap Ledger Live
  8. Tap back arrow to return to Connect MetaMask and note that QR is now animated
  9. In MetaMask tap the accounts drop down menu
  10. Tap + Add account or hardware wallet
  11. Tap Add hardware wallet
  12. Tap QR-based
  13. Grant camera permissions as requested
  14. Scan animated QR from Keystone into MetaMask
  15. Note progress bar is presented

Screenshots/Recordings

Before

image

After

image

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained:
    • What problem this PR is solving.
    • How this problem was solved.
    • How reviewers can test my changes.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@renfengshi renfengshi requested a review from a team as a code owner September 19, 2023 08:09
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 19, 2023 •
edited
Loading

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@renfengshi renfengshi force-pushed the add-progress branch 2 times, most recently from f2e177e to a2522df Compare September 19, 2023 08:25
@renfengshi
Copy link
Copy Markdown
Contributor Author

renfengshi commented Sep 19, 2023

I have read the CLA Document and I hereby sign the CLA

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 19, 2023 •
edited
Loading

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.21% 🎉

Comparison is base (6bddeba) 68.34% compared to head (d24f37f) 68.55%.

❗ Current head d24f37f differs from pull request most recent head 43a3503. Consider uploading reports for the commit 43a3503 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #20947      +/-   ##
===========================================
+ Coverage    68.34%   68.55%   +0.21%     
===========================================
  Files         1007     1006       -1     
  Lines        40254    40249       -5     
  Branches     10761    10761              
===========================================
+ Hits         27508    27589      +81     
+ Misses       12746    12660      -86
Files Changed Coverage Δ
.../components/app/qr-hardware-popover/base-reader.js 66.38% <80.00%> (+63.68%) ⬆️

... and 27 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gantunesr gantunesr added area-hardware team-accounts-framework Accounts team needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) hardware-qr labels Sep 20, 2023
gantunesr
gantunesr reviewed Sep 21, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM. Can you also add some tests?

@renfengshi renfengshi force-pushed the add-progress branch 6 times, most recently from d24f37f to 43a3503 Compare September 25, 2023 01:28
@renfengshi
Copy link
Copy Markdown
Contributor Author

renfengshi commented Sep 25, 2023

LGTM. Can you also add some tests?

@gantunesr The unit tests have been added.

@gantunesr
Copy link
Copy Markdown
Member

gantunesr commented Sep 26, 2023

@metamaskbot update-policies

@renfengshi renfengshi force-pushed the add-progress branch 2 times, most recently from 5346b09 to b535b74 Compare September 28, 2023 02:52
georgewrmarshall
georgewrmarshall requested changes Oct 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Looking good! Left one suggestion

@renfengshi renfengshi force-pushed the add-progress branch 2 times, most recently from 150fa01 to 7caadc1 Compare October 26, 2023 01:28
@renfengshi renfengshi force-pushed the add-progress branch 2 times, most recently from a8955c7 to 0249375 Compare November 23, 2023 03:48
@AlexJupiter AlexJupiter added team-hardware-wallets-deprecated DEPRECATED: please use "team-accounts-framework" instead and removed team-accounts-framework Accounts team labels Nov 28, 2023
@renfengshi renfengshi force-pushed the add-progress branch 3 times, most recently from 92fe33e to f98b7d0 Compare December 19, 2023 02:09
@renfengshi renfengshi force-pushed the add-progress branch 2 times, most recently from 06bee38 to 93d3d76 Compare December 22, 2023 10:14
@renfengshi
Copy link
Copy Markdown
Contributor Author

renfengshi commented Jan 8, 2024

@gantunesr @georgewrmarshall Is there anything I can do to push forward the merging of this PR?

georgewrmarshall
georgewrmarshall reviewed Jan 9, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

CSS LGTM! I don't have a QR code based hardware wallet so will need other folks to test

@FrederikBolding
Copy link
Copy Markdown
Member

FrederikBolding commented Jan 29, 2024

@renfengshi You will need to rebase this PR to get it passing CI. We had a faulty test that would break when run on PRs from forks. Rebasing should fix it!

@renfengshi
Copy link
Copy Markdown
Contributor Author

renfengshi commented Jan 29, 2024

@FrederikBolding All checks done.

@Akaryatrh Akaryatrh requested a review from gantunesr January 29, 2024 15:09
georgewrmarshall
georgewrmarshall approved these changes Jan 30, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

CSS LGTM

@gantunesr gantunesr removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) area-hardware hardware-qr labels Jan 30, 2024
@Akaryatrh Akaryatrh merged commit 93a950f into MetaMask:develop Jan 31, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2024
@metamaskbot metamaskbot added the release-11.11.0 Issue or pull request that will be included in release 11.11.0 label Feb 17, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 17, 2024

No release label on PR. Adding release label release-11.11.0 on PR, as PR was added to branch 11.11.0 when release was cut.

@metamaskbot metamaskbot added external-contributor INVALID-PR-TEMPLATE PR's body doesn't match template labels Feb 21, 2024
@angelcheung22 angelcheung22 added this to the Q1 2024 milestone Apr 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@georgewrmarshall georgewrmarshall georgewrmarshall approved these changes

@gantunesr gantunesr gantunesr approved these changes

Assignees

No one assigned

Labels

external-contributor INVALID-PR-TEMPLATE PR's body doesn't match template release-11.11.0 Issue or pull request that will be included in release 11.11.0 team-hardware-wallets-deprecated DEPRECATED: please use "team-accounts-framework" instead

Projects

None yet

Milestone

Q1 2024 (CET hw wallet)

Development

Successfully merging this pull request may close these issues.

8 participants

@renfengshi @gantunesr @FrederikBolding @metamaskbot @georgewrmarshall @Akaryatrh @AlexJupiter @angelcheung22