Adding popular custom network integration #14557
Merged
Conversation
This file contains hidden or 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
Contributor
|
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. |
filipsekulic
force-pushed
the
integrate-flow-adding-custom-network
branch
from
2e85a18 to
d355d10
Compare
filipsekulic
force-pushed
the
integrate-flow-adding-custom-network
branch
from
ca65009 to
18c1cb9
Compare
Collaborator
filipsekulic
changed the title
Collaborator
Builds ready [0b58edf]Page Load Metrics (1750 ± 32 ms)
highlights:storybook
|
Contributor
|
Verified by QA |
Collaborator
Builds ready [0b83142]Page Load Metrics (1704 ± 39 ms)
highlights:storybook
|
Contributor
Author
Collaborator
Builds ready [c36f1a8]Page Load Metrics (1818 ± 67 ms)
highlights:storybook
|
darkwing
reviewed
May 13, 2022
app/_locales/en/messages.json
Outdated
| "message": "Yes, let's try" | ||
| }, | ||
| "youHaveAddedAll": { | ||
| "message": "You've added all the popular networks. You can discover more networks $1 Or you can $2" |
Contributor
There was a problem hiding this comment.
We need a description key here to describe what $1 and $2 represent.
darkwing
reviewed
May 13, 2022
darkwing
reviewed
May 13, 2022
|
|
||
| &__button { | ||
| display: inline !important; | ||
| padding: 0 !important; |
Contributor
There was a problem hiding this comment.
There are a lot of !important usages in this file. Can these be avoided with a parent className?
darkwing
reviewed
May 13, 2022
| /** | ||
| * Icon location | ||
| */ | ||
| leftIconUrl: PropTypes.string, |
Contributor
There was a problem hiding this comment.
Should we default leftIconUrl to ''?
Contributor
|
This is how it looks in I think some of the CSS added in this PR could be changed from |
Contributor
georgewrmarshall
left a comment
georgewrmarshall
left a comment
There was a problem hiding this comment.
Looking really good nice work using the 8px grid values for bounding box values(margin, padding). Made a couple small suggestions.
Not sure if this is a biggy? Some warning appears about the history stack when a network is deleted
warning.history.mov
This might be an existing prop type error but it would be good to fix:
Comment on lines
97
to
106
| key="link" | ||
| className="add-network__edge-case-box__link" | ||
| href="https://chainlist.wtf/" | ||
| target="_blank" | ||
| rel="noreferrer" | ||
| > | ||
| {t('here')}. | ||
| </a>, | ||
| <Button | ||
| key="link" |
Contributor
There was a problem hiding this comment.
Keys should have different ids
Suggested change
| key="link" | |
| className="add-network__edge-case-box__link" | |
| href="https://chainlist.wtf/" | |
| target="_blank" | |
| rel="noreferrer" | |
| > | |
| {t('here')}. | |
| </a>, | |
| <Button | |
| key="link" | |
| key="link" | |
| className="add-network__edge-case-box__link" | |
| href="https://chainlist.wtf/" | |
| target="_blank" | |
| rel="noreferrer" | |
| > | |
| {t('here')}. | |
| </a>, | |
| <Button | |
| key="button" |
| </Box> | ||
| } | ||
| trigger="mouseenter" | ||
| theme={theme === THEME_TYPE.DEFAULT ? 'light' : 'dark'} |
Contributor
There was a problem hiding this comment.
Doesn't look like THEME_TYPE object has a DEFAULT key which is why the tooltip is still showing in dark mode. @darkwing should we add a DEFAULT key to the THEME_TYPE object? Then this will work as expected.
Before
After
After adding DEFAULT to THEME_TYPE
Contributor
There was a problem hiding this comment.
Still looks like it's using the wrong text color here
Contributor
Author
There was a problem hiding this comment.
Good catch! 💯
Thanks! 😊
I have not seen there was the OS theme option, and that was the part I was missing...
You can check it again and see the changes: 8986581
Collaborator
Builds ready [dc67e85]Page Load Metrics (1783 ± 53 ms)
highlights:storybook
|
Collaborator
Builds ready [8986581]Page Load Metrics (1835 ± 53 ms)
highlights:storybook
|
0ex-d
suggested changes
May 21, 2022
Comment on lines
+46
to
+48
| if (child?.hide === true) { | ||
| return allChildren; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (child?.hide === true) { | |
| return allChildren; | |
| } | |
| if (child?.hide) { | |
| return allChildren; | |
| } |
Comment on lines
132
to
135
| if (getEnvironmentType() === ENVIRONMENT_TYPE_POPUP) { | ||
| global.platform.openExtensionInBrowser(ADD_NETWORK_ROUTE); | ||
| this.props.history.push(ADD_POPULAR_CUSTOM_NETWORK); | ||
| } else { | ||
| this.props.history.push(ADD_NETWORK_ROUTE); | ||
| this.props.history.push(ADD_POPULAR_CUSTOM_NETWORK); |
There was a problem hiding this comment.
why do we need a condition here if we can just pass this.props.history.push(ADD_POPULAR_CUSTOM_NETWORK);
Contributor
There was a problem hiding this comment.
That's true. @filipsekulic I guess the if/else can be removed here?
| } | ||
|
|
||
| function getValues(pendingApproval, t, actions) { | ||
| const originIsMetaMask = pendingApproval.origin === 'metamask'; |
| {t('someNetworksMayPoseSecurity')}{' '} | ||
| <a | ||
| key="zendesk_page_link" | ||
| href="https://metamask.zendesk.com/hc/en-us/articles/4417500466971" |
There was a problem hiding this comment.
Can this URL be put in a constant mapping file
Collaborator
Builds ready [5d2b555]Page Load Metrics (1769 ± 63 ms)
highlights:storybook
|
Collaborator
Builds ready [f551831]Page Load Metrics (1894 ± 61 ms)
highlights:storybook
|
georgewrmarshall
requested changes
May 26, 2022
Contributor
There was a problem hiding this comment.
Can you rebase from develop please @filipsekulic? There are some UI updates that might affect this page
Also can we make this header the same as this one so it's consistent?
This header:
should look like this:
Then this header should probably be updated
Contributor
|
Also noticed the token icons get squashed when the network name is longer |
Collaborator
Builds ready [13d5550]Page Load Metrics (1647 ± 41 ms)
highlights:storybook
|
filipsekulic
force-pushed
the
integrate-flow-adding-custom-network
branch
from
57f24d9 to
a17ecfa
Compare
Collaborator
Builds ready [a17ecfa]Page Load Metrics (2054 ± 110 ms)
highlights:storybook
|
georgewrmarshall
approved these changes
Jun 22, 2022
Contributor
georgewrmarshall
left a comment
georgewrmarshall
left a comment
There was a problem hiding this comment.
UI LGTM! Nice work 💯 Could you please add a PR description and "Manual Testing Steps" it helps the support content folks write documentation
- verified adding / removing networks
- verified UI in light/dark mode
- verified responsive layout
- verified flow in popover/action view
Contributor
Author
|
Hey @georgewrmarshall! Thank you! |
Contributor
|
@seaona @georgewrmarshall re: Capitalization, we should be using sentence case throughout |
Contributor
|
Thanks @coreyjanssen! I've created an issue #15039 |
danjm
approved these changes
Jun 29, 2022
darkwing
approved these changes
Jun 30, 2022
Contributor
darkwing
left a comment
darkwing
left a comment
There was a problem hiding this comment.
Approved.
From a UX perspective, I wish we didn't send the user home, in the case that they want to add a handful of networks, but this is a huge new feature we should get out there ASAP. Well done!
Heey @darkwing, thank you for this UX feedback here! That was indeed the original design, but we learned during user testing that users were confused when they were brought back to the network list + from MetaMetrics data we found out that not that many users have multiple networks added (Of all users that have added custom networks in MM, 75% of them have only added 1 network) so we decided to change the original behavior to this. |
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.
Explanation
Created a new feature for adding the popular custom networks. When enabled from the
Experimentaltab, this feature allows users to add one of the popular custom networks like Arbitrum One, Polygon Mainnet etc.More Information
Fixes: #12903
Screenshots/Screencaps
Screen.Recording.2022-06-23.at.10.31.33.mov
Manual Testing Steps
Settings.Experimentaltab.Show Custom Network List.Networkstab within theSettingsand then to click on the buttonAdd a network.Add Network.back (<)button which takes the user to theNetworksscreen. Also, the networks which rely on third parties have the exclamation triangle next to their name which can be hovered and then the tooltip with information shows up with a linkLearn morewhich takes the user to the web-pageThe risks of connecting to an unknown network.NOTE: If the button
Add a network manuallyfrom the app footer is clicked, then the screen for manually adding the network is shown and from there the network can be added by entering the appropriate information about the network.Addbutton. When clicked, the popup with the network information shows up. This popup hastooltips (i)which show information when hovered. Clicking on thescams and network security riskstakes the user to the web-pageUser Guide: Custom networks and sidechains. If a buttonView all detailsis clicked then the new, smaller popup shows up with the information about the network and this popup can be closed by simply clicking on theClosebutton orX.Cancelbutton which rejects the pending approval for adding the network and the user stays on the same page. If theApprovebutton is clicked, then the network will be added, the user will be taken to theHome pageand the new popup will show up.Switch to network_nameor click on theDismissbutton and later switch to that network.