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

Refactor Confirm components #3824

Closed
danjm opened this issue Apr 2, 2018 · 2 comments
Closed

Refactor Confirm components #3824

danjm opened this issue Apr 2, 2018 · 2 comments
Labels
area-UI Relating to the user interface.
Milestone

Comments

@danjm
Copy link
Contributor

danjm commented Apr 2, 2018

Currently the confirm component is split into three. This was only meant to be a temporary measure while we were still developing the new ui send component. The intent was to
(a) simplify some more complicated and less readable code that there at the time, and
(b) get a clear picture of the different needs that the confirm component is meant to fulfill

We now need to refactor this to minimize code duplication, maintenance costs and inconsistent behaviour bugs. My proposed refactor is:

  • replace pending-tx/index.js and conf-tx.js with a proper container component (which is the purpose it currently fills)
  • create one view component that can accomodate different views and actions based on what is mapped to it in the controller component
  • plus the points for refactor the send component: Refactor send component #3725
  • full unit tests

Before we do this it is probably a good idea to gather requirements for potential future transaction types the confirm component will have to accomodate (e.g. for different erc20 token methods, or methods from other token standards). While we will not add support for these during this refactor, understanding requirements can help us pave the way for those additions during this refactor.

@danjm
Copy link
Contributor Author

danjm commented Apr 10, 2018

Follow the outline for directory structure and container and component design that is included in the comment here: #3725

@danjm danjm added the area-UI Relating to the user interface. label Apr 10, 2018
@danjm danjm added this to the Sprint 04 milestone Apr 10, 2018
@cjeria
Copy link
Contributor

cjeria commented May 23, 2018

@danjm I'm on the same page as you. In order to design for future transaction types, understanding requirements and their use cases is key.

I've create a design issue that aims to address how to think about ETH-less transaction types (pure function calls) very generically MetaMask/Design#16

This above issue stems from feedback @danfinlay provided in a newly proposed Eth Transfer confirm screen design in this issue MetaMask/Design#11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-UI Relating to the user interface.
Projects
None yet
Development

No branches or pull requests

4 participants