Skip to content

Standardize network settings page#9740

Merged
rekmarks merged 14 commits intodevelopfrom
standardize-network-settings
Oct 29, 2020
Merged

Standardize network settings page#9740
rekmarks merged 14 commits intodevelopfrom
standardize-network-settings

Conversation

@rekmarks
Copy link
Copy Markdown
Member

@rekmarks rekmarks commented Oct 27, 2020

Summary

Our network settings page has been janky for a long time. This PR attempts to update its UI such that it's inline with our other settings pages. This was accomplished by the creation of an additional route used only in the popup, for the network form.

My sense is that the CSS of this page is still probably less than great, but it ought to do for now. Reviewers should pay extra attention to my use of routes and history, since I'm inexperienced with react-router-dom.

These changes mostly affect the popup view; the fullscreen view should be largely unaffected.

Changes in Detail

  • The browser back functionality now works correctly with the network form
    • I also added the settings page header back arrow to the networks settings, and it works
  • The network settings subheader was removed in the popup
  • The "Add Network" button was added to a sticky footer in the networks settings page
  • Network form buttons are now hidden when the form is viewOnly
  • Network settings routes were updated
    • There's a new route for the network form, which is only used in the popup
    • The fullscreen UI always renders the form, and the new route is never used there

Testing Steps

  • Open the network form, in fullscreen and the popup
  • Try to break it

Screenshots / Recordings

Before
After
After (Recording) Note: The weird shadows that stick around are just an artifact of the recording.

@rekmarks rekmarks added DO-NOT-MERGE Pull requests that should not be merged and removed DO-NOT-MERGE Pull requests that should not be merged labels Oct 27, 2020
@rekmarks rekmarks requested a review from rachelcope October 28, 2020 23:17
@rekmarks rekmarks marked this pull request as ready for review October 28, 2020 23:17
@rekmarks rekmarks requested a review from a team as a code owner October 28, 2020 23:17
@rekmarks rekmarks requested a review from danjm October 28, 2020 23:17
Comment on lines 206 to 210
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This spares us from re-pushing NETWORKS_ROUTE unnecessarily. Used in NetworkForm.componentWillUnmount.

Comment on lines 38 to 39
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The default state is '', and resetting it on unmount works equally well.

@rekmarks rekmarks force-pushed the standardize-network-settings branch from b668e20 to 59a38a2 Compare October 29, 2020 06:34
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [59a38a2]
Page Load Metrics (560 ± 80 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30113562512
domContentLoaded26582655816680
load26782756016680
domInteractive26582655716579

@brad-decker brad-decker self-assigned this Oct 29, 2020
@brad-decker brad-decker requested review from brad-decker and removed request for danjm October 29, 2020 17:02
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [ab80832]
Page Load Metrics (518 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30112472211
domContentLoaded31885451617886
load31985551817986
domInteractive31785451517886

Copy link
Copy Markdown
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM!

@brad-decker
Copy link
Copy Markdown
Contributor

Works like a charm on my machine. 👍

Copy link
Copy Markdown

@rachelcope rachelcope left a comment

Choose a reason for hiding this comment

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

This looks great. Great improvements to make it consistent with the rest of the settings pages

@rekmarks rekmarks merged commit 7a90766 into develop Oct 29, 2020
@rekmarks rekmarks deleted the standardize-network-settings branch October 29, 2020 20:17
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2020
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.

4 participants