Choose accounts refactor #13039
Merged
Choose accounts refactor #13039
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
Builds ready [ccfa4a8]Page Load Metrics (1015 ± 42 ms)
highlights:storybook
|
georgewrmarshall
previously approved these changes
Dec 10, 2021
There was a problem hiding this comment.
UI looks good. There are a few rogue values here that don't adhere to the 8px grid convention but I think this is the UI that I myself tried to refactor and had a very hard time as the different components effect the layout of different pages. So for the sake of keeping things consistent. I'm happy to approve.
darkwing
reviewed
Dec 10, 2021
| flex: 0; | ||
| margin-top: 36px; | ||
| width: 100%; | ||
| padding-left: 15px; |
There was a problem hiding this comment.
Let's use padding-inline-start and padding-inline-end for better RTL support.
There was a problem hiding this comment.
We can use margin-inline-start and margin-inline-end elsewhere in this file.
There was a problem hiding this comment.
Pushed up some changes, please let me know if that works.
|
One thing that's not apart of this PR but might be a good change is if we use sentence case for "Connect with MetaMask" in |
rekmarks
requested changes
Dec 10, 2021
Builds ready [f52d5a7]Page Load Metrics (977 ± 16 ms)
highlights:storybook
|
Builds ready [f52d5a7]Page Load Metrics (1071 ± 35 ms)
highlights:storybook
|
hmalik88
force-pushed
the
choose-accounts-refactor
branch
from
f52d5a7
to
91e7c4b
Compare
Builds ready [91e7c4b]Page Load Metrics (1134 ± 30 ms)
highlights:storybook
|
shanejonas
reviewed
Dec 13, 2021
georgewrmarshall
requested changes
Dec 13, 2021
There was a problem hiding this comment.
Looks like isn't receiving the correct data after the refactor.
Testing on E2E Test Dapp
Tested on uniwap as well
…n choose account story
…title, removed redundant account list story
This reverts commit ccfa4a8.
…ctType to targetSubjectMetadata
hmalik88
force-pushed
the
choose-accounts-refactor
branch
from
91e7c4b
to
ab59b4a
Compare
Builds ready [2d38d99]Page Load Metrics (1167 ± 51 ms)
highlights:storybook
|
|
Wow, really excellent work @hmalik88 ! |
Builds ready [262edb2]Page Load Metrics (1232 ± 62 ms)
highlights:storybook
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to: Snaps Installation UI issue
Background Updates: Fixes to
subjectMetadatawere made to include theorigin. An "unknown" subject type was added and integrated into theuseOriginMetadatahook. Background updates are being made in this PR instead of a separate one, because in interest of time, the changes are relevant to this UI and because there aren't a significant number of changes.UI Explanation: Flask designs (Figma) updated the
ChooseAccountscreen to have a bolded title and black subtitle. The account list was refactored into its own component (AccountList) in an effort to tidy upChooseAccount. A wrapper div was added around the account list to prevent storybook from collapsing the div.ChooseAccountwas also converted to a functional component. Following pics are of the collapsed account list in storybook before the wrapper, a before of it functioning and after of when the styles/text were updated:Note:
ChooseAccountwill be updated in the future to change the choose accounts text to conditionally end with either "site" or "snap" when snaps supportseth_requestAccounts.Manual testing steps:
ChooseAccountyarn start, load the extension into chrome, navigate to a dapp (i.e. Aave) and connect to MetaMask.