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

Add delete to custom rpc form #6650

Closed
wants to merge 4 commits into from
Closed

Conversation

chikeichan
Copy link
Contributor

delrpc

@chikeichan chikeichan requested review from danjm and whymarrh as code owners May 23, 2019 08:16
@danjm
Copy link
Contributor

danjm commented May 23, 2019

Why the cancel button in the form, as opposed to a button next to the list item?

And I guess there is a technical reason we can't delete a network that is currently selected? Or rather, do you have any sense of the scope and implications of such a change?

@danjm
Copy link
Contributor

danjm commented May 23, 2019

The code is good here.

My main question is the UX choice of a form button, instead of a button on the list item.

@chikeichan
Copy link
Contributor Author

  • UX choice was subjective - i thought the secondary button offer a clearer CTA than an X on hover, unless the X on hover is already part of the design (which i assumed it wasn't) @cjeria could you please take a look?

And I guess there is a technical reason we can't delete a network that is currently selected? Or rather, do you have any sense of the scope and implications of such a change?

I was basically mirroring the behavior on the network dropdown. The technical complexity to do this is fairly low. It is more about not knowing which network to switch to after the user delete the network they are currently on. We can always default fallback to mainnet, but that seems to be suboptimal since it's not an explicit network switch

@cjeria
Copy link
Contributor

cjeria commented Jun 5, 2019

Looking good!

Some feedback:

  • Creating a new network should show the "new network" label in the network list at the bottom
  • When a new network is created but not saved, disabled the "new network" button and the save button (below the network form elements).
    -- Add a cancel button to cancel out of the new unsaved network.
  • After filling out the required network fields, enable the save button at the bottom of the fields
  • When the new network is saved
    -- enable the new network button again
    -- Add a "delete network" button below the new network fields (next to the disabled cancel and save button).
  • The form elements are very tall, would be good to create a large input and a regular input variables.
  • Increase spacing between the input section (title + input) per the design system (24px).

Feedback video

I also recorded a feedback video w/prototype which I thought would be easier to explain the many points above.
https://www.loom.com/share/dcee3a6733dd4b67a4143a916f5b1bfa

Visual bug

Is this just on my end?
image

Prototype

Link to figma prototype
https://www.figma.com/proto/cl052ly5iSXnqQqGU4V84s7A/Settings?node-id=66%3A1108&viewport=-1035%2C954%2C0.2857494652271271&scaling=min-zoom

@chikeichan let me know if you have any Qs!

@danjm
Copy link
Contributor

danjm commented Jun 14, 2019

@chikeichan I fixed lint errors and updated e2e tests on two commits that can be found on this branch: https://github.com/MetaMask/metamask-extension/compare/delrpc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants