Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

Add Max button to send the whole balance #302

Merged
merged 19 commits into from
Dec 20, 2018
Merged

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Dec 16, 2018

closes #254

TODO:
1. Understand why the max button actually submits the form
2. Understand this
3. Style the button. More on this later (using this would be great but I couldn't manage to make it work so far)
4. Test with ERC20

I need help regarding 1. and 2. @axelchalon, @amaurymartiny or @yjkimjunior whoever has time :)

Looks like this:
image

it acts as a toggle button, the gas and address can be changed and the amount will update accordingly, if max is selected, the amount field is disabled.

@Tbaut Tbaut changed the title WIP: Add Max button to send the whole balance Add Max button to send the whole balance Dec 17, 2018
@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 17, 2018

Happy to get some more ideas about the look&feel.

  • I found the usual "blue" used for buttons too loud, so I chose black instead.
  • I'm not 100% a fan of the fact that amount is not centred anymore but didn't find anything better so that the amount field remains large. We surely can find something better.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 17, 2018

TODO:
Need to add an error when clicking on "max" while not having any ETH (or not enough to even pay for the gas) as it will result in a negative amount.

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Works well, some code needs change.

At first the amount not being in the middle also felt a bit weird. Idea: put the max button smaller, cf #38? So that amount still seems in the middle.

packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 18, 2018

I couldn't find a satisfying position at the bottom right, so I centred it as well and find it better:

When it's not active
image

Once the user has clicked the max, the amount is then disabled.
image

@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 19, 2018

Another suggestion:

image

I tried without the border so that it feels a little lighter, but then it doesn't look like a button anymore.

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

I like the small button design

packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved
packages/fether-react/src/Send/TxForm/TxForm.js Outdated Show resolved Hide resolved

if (token.address === 'ETH') {
output = fromWei(
toWei(balance).minus(gasBn.mul(toWei(gasPriceBn, 'shannon')))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to merge #274 first, because the syntax is changed to .multipliedBy

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure go ahead

@Tbaut
Copy link
Collaborator Author

Tbaut commented Dec 20, 2018

on ice until we merge #274

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm!

@amaury1093
Copy link
Collaborator

Will rebase #274 on top of this.

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

Successfully merging this pull request may close these issues.

Add a max toggle button to send the whole account amount
2 participants