-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: decouple account selector from qr code connector #8093
Conversation
In order to enable account selector being reusable, we have to decouple the account selector from QR hardware wallet the changes including decouple account selector & account detail components in addtional we also add: An hook useBlockExplorer for generate the block explorer redirect method An hook useAccountsBalance for generate the account balance mapping
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. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8093 +/- ##
==========================================
+ Coverage 40.32% 40.38% +0.05%
==========================================
Files 1235 1239 +4
Lines 29948 29956 +8
Branches 2875 2874 -1
==========================================
+ Hits 12078 12098 +20
+ Misses 17175 17160 -15
- Partials 695 698 +3 ☔ View full report in Codecov by Sentry. |
|
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d99f912b-acb1-4d28-b3d3-daa05736eafe |
E2E Manual Test:- Screen_Recording_20231215_111019_MetaMask.mp4 |
I found a minor bug unrelated to this PR, it'd be good to open a ticket for it cc @angelcheung22 Edit: When you navigate to Etherscan from the "Connect QR Hardware Account" screen, it dismisses the previous screen to navigate to Etherscan. I think the desired behaviour is to be able to navigate back to the "Connect QR Hardware Account". RPReplay_Final1705930225.mp4 |
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.
The rest of the code looks good to me
Hey Gustavo, im confused about the bug that you are showing in the video. May I know whats the expected steps? |
Co-authored-by: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com>
|
i have implemented two unit tests for now This PR is ready to review again. |
Regarding the small bug @gantunesr report, i have tried with |
https://app.zenhub.com/workspaces/cet-metamask-hardware-wallets-6566ee6aedc46007d5a260bb/issues/zh/139 <-per discussion from last weekly with alex, this one can be low priority and add to backlog for future fixing. Added new ticket in zenhub |
Description
In order to enable multiple account select in ledger hardware wallet,
This PR is focus on decoupling the
Account selector component
from QR hardware wallet for reusable purposeThe changes should not impact to any UI / operations differences compare to existing QR hardware wallet account selector
For clean code purpose, the changes also decoupling the method
toBlockExplorer
from account selector component into a new hookuseBlockExplorer
Logical flow
![image](https://private-user-images.githubusercontent.com/102275989/290485526-ef5715e1-2603-49f0-96e8-6c2c394fb9a5.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjE5Nzk1MzYsIm5iZiI6MTcyMTk3OTIzNiwicGF0aCI6Ii8xMDIyNzU5ODkvMjkwNDg1NTI2LWVmNTcxNWUxLTI2MDMtNDlmMC05NmU4LTZjMmMzOTRmYjlhNS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwNzI2JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDcyNlQwNzMzNTZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT01MDE0OTk2MGRhZjNlYTIzYWI4MzZjZmMzYjE5YjVmZjk4N2ZhNTFhNjlmYjBiMDMwNmE3YzQ1YmI2OGE5OThiJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.Do42sJ52raRbE9y-7xfuUyYb06cR683eGtztfmQE-Eg)
Related issues
issue : 109
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist