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

[5.3] [IMPROVEMENT] Update SelectQRAccounts UI #4070

Merged
merged 29 commits into from
May 26, 2022
Merged

Conversation

gantunesr
Copy link
Member

Description

This PR updates the style of the SelectQRAccounts component and refactors the HOC withQRHardwareAwareness

Use Case

GIVEN I'm importing my accounts from my Keystone to MMM
WHEN I select an account or more
THEN the account(s) should be imported to MMM

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Progresses #3952

@gantunesr gantunesr added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 11, 2022
@gantunesr gantunesr requested a review from a team as a code owner April 11, 2022 18:33
@plasmacorral
Copy link
Contributor

Why do we have specific formatting for the iphone X in app/components/Views/ConnectQRHardware/SelectQRAccounts/index.tsx and app/components/Views/ConnectQRHardware/index.tsx, @soralit?

@plasmacorral plasmacorral added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 18, 2022
@plasmacorral plasmacorral self-assigned this Apr 18, 2022
@plasmacorral
Copy link
Contributor

plasmacorral commented Apr 21, 2022

This is how the screen is presented on my Pixel 3aScreenshot_20220421-151936.png

And for reference how this looked in earlier builds: Screenshot_20220408-104836.png

The way we are wrapping the public address on a screen of this size is not ideal. edited to update-I later realized that the characters in the second line were the eth balance rather than the address. Should we preserve the unit "ETH" or was that omission intentional?.

I must defer to design, but it might be nice to display "Keystone 0" on this screen to be consistent with how account names will be presented in the account menu after import, rather than just "0" as shown currently.

@plasmacorral
Copy link
Contributor

Just following up on my comment above:

...wrapping the public address on a screen of this size...

I now see that it is the Eth balance appearing below the public address (not the address wrapping to a new line).

Should we preserve the unit "ETH" to make this more clear, or was that removed intentionally?

@gantunesr
Copy link
Member Author

Should we preserve the unit "ETH" to make this more clear, or was that removed intentionally?

It was a bug, its already fixed in the last commit

I must defer to design, but it might be nice to display "Keystone 0" on this screen to be consistent with how account names will be presented in the account menu after import

I added the text "QR Hardware" as its showed in the Wallet View. What do you think @holantonela and @plasmacorral?

In the following screenshots you can see the evidence,

drawing

@gantunesr gantunesr requested a review from owencraston May 6, 2022 04:12
Copy link
Contributor

@owencraston owencraston left a comment

Choose a reason for hiding this comment

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

Nice work. Only thing I would say is we should try and move withQRHardwareAwareness to a hook but i'd suggest we do this in another PR to reduce the scope. I didn't run the app so this is just a code review!

@plasmacorral plasmacorral added QA in Progress QA has started on the feature. QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA in Progress QA has started on the feature. labels May 11, 2022
@plasmacorral
Copy link
Contributor

Android:
Screen Shot 2022-05-11 at 4.20.15 PM.png
Screen Shot 2022-05-11 at 4.18.05 PM.png

iOS:
Screen Shot 2022-05-11 at 4.19.10 PM.png
Screen Shot 2022-05-11 at 4.21.16 PM.png

@plasmacorral plasmacorral added QA in Progress QA has started on the feature. and removed QA Passed A successful QA run through has been done labels May 12, 2022
@gantunesr gantunesr removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 13, 2022
@plasmacorral plasmacorral added QA Passed A successful QA run through has been done QA in Progress QA has started on the feature. and removed QA in Progress QA has started on the feature. QA Passed A successful QA run through has been done labels May 16, 2022
@AlexJupiter
Copy link

Candidate for 5.3

@AlexJupiter AlexJupiter changed the title [IMPROVEMENT] Update SelectQRAccounts UI [5.3] [IMPROVEMENT] Update SelectQRAccounts UI May 26, 2022
@gantunesr gantunesr merged commit 66ed569 into main May 26, 2022
@gantunesr gantunesr deleted the improv/keystone branch May 26, 2022 15:45
@github-actions github-actions bot locked and limited conversation to collaborators May 26, 2022
@mobularay mobularay added the release-5.3.0 Issue or pull request that will be included in release 5.3.0 label Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-hardware QA Passed A successful QA run through has been done release-5.3.0 Issue or pull request that will be included in release 5.3.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants