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

Opening Network Forms in full screen from the popup view #15442

Merged
merged 1 commit into from Aug 3, 2022
Merged

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Aug 3, 2022

Fixes: #15399

This restores the behavior of opening the Network form in full view when attempting to edit one via the popup window.

Testing involves a simple regression check in the full screen view and ensuring the aformentioned behavior in the popup

@ryanml ryanml requested a review from danjm August 3, 2022 02:51
@ryanml ryanml requested a review from a team as a code owner August 3, 2022 02:51
@ryanml ryanml self-assigned this Aug 3, 2022
@metamaskbot
Copy link
Collaborator

Builds ready [a768797]
Page Load Metrics (1744 ± 81 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint832027280520250
domContentLoaded15432032172415474
load15552123174416881
domInteractive15432032172415474

highlights:

storybook

@@ -68,7 +66,7 @@ const NetworksListItem = ({
setSearchedNetworks([]);
dispatch(setSelectedSettingsRpcUrl(rpcUrl));
if (!isFullScreen) {
history.push(NETWORKS_FORM_ROUTE);
global.platform.openExtensionInBrowser(NETWORKS_ROUTE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why NETWORKS_ROUTE instead of NETWORKS_FORM_ROUTE?

Copy link
Contributor

Choose a reason for hiding this comment

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

const NETWORKS_ROUTE = '/settings/networks';
const NETWORKS_FORM_ROUTE = '/settings/networks/form';

Went down a bit of a rabbit hole here. As the issue shows, the route, settings/networks/form, resolves to the generic settings page. From my investigation, it appears the path is not resolving to the networks tab properly and from there, the shouldRenderNetworkForm value when the route is NETWORKS_FORM_ROUTE is not being set.

hardcoding the route to resolve to the networks tab and setting shouldRenderNetworkForm to true shows this popup page:
Screenshot 2022-08-03 at 8 53 17 PM

I think that applying the current changes now would make NETWORKS_FORM_ROUTE obsolete which might not be what we want.

p.s. i deleted the last copy of this comment and reposted this comment with the proper reference to see if I can remove the reference to the incorrect issue ticket

Copy link
Contributor

Choose a reason for hiding this comment

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

p.s.p.s. the reference to this PR from the unrelated issue was not removed when I deleted the comment I mentioned above

@PeterYinusa PeterYinusa added this to the v10.18.2 milestone Aug 3, 2022
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

As a quick fix, this seems okay to merge. If we do merge this, we should follow up to fix the networks route and forms UI in the popup view, or fully remove this feature from the popup by removing the code related to the NETWORKS_FORM_ROUTE

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

we need to resolve the route question

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

actually, I think it has been appropriately answered, and this PR is good for now

@danjm danjm merged commit 97b10f9 into develop Aug 3, 2022
@danjm danjm deleted the fix-15399 branch August 3, 2022 15:37
@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 2022
@digiwand digiwand restored the fix-15399 branch August 5, 2022 20:07
@digiwand digiwand deleted the fix-15399 branch January 16, 2024 16:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to edit a network via the popup, only works on full screen
6 participants