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

Send Page amount USD. #152

Merged
merged 18 commits into from
Jun 30, 2020
Merged

Conversation

Sirmorrison
Copy link
Contributor

@Sirmorrison Sirmorrison commented Jun 14, 2020

Resolves

Issue #142

What's new

This PR adds the send amount USD equivalence for all transactions fields to the send Page.

Relevant screenshots or logs

image

image

image

@Sirmorrison Sirmorrison marked this pull request as ready for review June 17, 2020 07:25
@oshorefueled
Copy link
Contributor

The amount-usd feature doesn't work. The currency conversion feature isn't just to show a user DCR equivalent in dollars but to also allow the user input an amount in dollars and have its DCR equivalent sent. Currently, it doesn't do that.

@oshorefueled
Copy link
Contributor

The position of the conversion button and values look off, we should be able to make it look just like the mobile. Kindly have a look at the related issue for the mobile mockup.

Screenshot 2020-06-19 at 7 24 41 AM

@oshorefueled oshorefueled added this to In Progress in godcr board via automation Jun 19, 2020
@Sirmorrison
Copy link
Contributor Author

The position of the conversion button and values look off, we should be able to make it look just like the mobile. Kindly have a look at the related issue for the mobile mockup.

Screenshot 2020-06-19 at 7 24 41 AM

Resolved.

ezgif com-video-to-gif(9)

@oshorefueled oshorefueled moved this from In Progress to Review in progress in godcr board Jun 22, 2020
ui/send_page.go Outdated Show resolved Hide resolved
ui/send_page.go Outdated Show resolved Hide resolved
ui/send_page.go Outdated Show resolved Hide resolved
ui/send_page.go Outdated Show resolved Hide resolved
wallet/commands.go Outdated Show resolved Hide resolved
ui/send_page.go Outdated
Comment on lines 715 to 720
pg.activeTotalAmount = dcrutil.Amount(amountAtoms).String()
pg.inactiveTotalAmount = totalAmountUSDTostring
pg.activeTransactionFeeValue = dcrutil.Amount(txFee).String()
pg.inactiveTransactionFeeValue = fmt.Sprintf("(%f USD)", txFeeValueUSD)
pg.activeTotalCostValue = dcrutil.Amount(totalCostDCR).String()
pg.inactiveTotalCostValue = fmt.Sprintf("(%s USD)", strconv.FormatFloat(totalAmountUSD+txFeeValueUSD, 'f', 7, 64))
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks repeated. To make it cleaner, you can create a data type for it. For each case, assign the respective data properties their respective methods and pass that to a single method that initiates those values to the SendPage properties.
Also, all that would be cleaner if they were in a separate method.

ui/send_page.go Outdated
return amountAtoms
}

func (pg *SendPage) setUSDValues(usdExchangeRate, inputAmount float64, totalCostDCR, txFee, amountAtoms int64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

make this a data type and pass a single argument to the Set method.

ui/send_page.go Outdated
Comment on lines 746 to 751
pg.activeTotalAmount = totalAmountUSDTostring
pg.inactiveTotalAmount = dcrutil.Amount(amountAtoms).String()
pg.activeTransactionFeeValue = fmt.Sprintf("%f USD", txFeeValueUSD)
pg.inactiveTransactionFeeValue = fmt.Sprintf("(%s)", dcrutil.Amount(txFee).String())
pg.activeTotalCostValue = fmt.Sprintf("%s USD", strconv.FormatFloat(inputAmount+txFeeValueUSD, 'f', 7, 64))
pg.inactiveTotalCostValue = fmt.Sprintf("(%s )", dcrutil.Amount(totalCostDCR).String())
Copy link
Contributor

Choose a reason for hiding this comment

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

you only need one method to set these.

ui/send_page.go Outdated
Comment on lines 761 to 766
pg.activeTotalAmount = dcrutil.Amount(amountAtoms).String()
pg.inactiveTotalAmount = totalAmountUSDTostring
pg.activeTransactionFeeValue = dcrutil.Amount(txFee).String()
pg.inactiveTransactionFeeValue = fmt.Sprintf("(%f USD)", txFeeValueUSD)
pg.activeTotalCostValue = dcrutil.Amount(totalCostDCR).String()
pg.inactiveTotalCostValue = fmt.Sprintf("(%s USD)", strconv.FormatFloat(totalAmountUSD+txFeeValueUSD, 'f', 7, 64))
Copy link
Contributor

Choose a reason for hiding this comment

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

you only need one method to set these.

ui/send_page.go Outdated
Comment on lines 769 to 776
func (pg *SendPage) updateDefaultValues(totalCostDCR, txFee, amountAtoms int64, noExchangeText string) {
pg.activeTotalAmount = dcrutil.Amount(amountAtoms).String()
pg.inactiveTotalAmount = noExchangeText
pg.activeTransactionFeeValue = dcrutil.Amount(txFee).String()
pg.inactiveTransactionFeeValue = ""
pg.activeTotalCostValue = dcrutil.Amount(totalCostDCR).String()
pg.inactiveTotalCostValue = ""
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the only method you need to set the data. And it only really needs to accept one argument.

ui/send_page.go Outdated Show resolved Hide resolved
godcr board automation moved this from Review in progress to Reviewer approved Jun 30, 2020
@oshorefueled oshorefueled merged commit f2db3ea into planetdecred:master Jun 30, 2020
godcr board automation moved this from Reviewer approved to Done Jun 30, 2020
song50119 pushed a commit to song50119/godcr that referenced this pull request Apr 24, 2022
* add default value without exchange

* add currency change button

* remove imported account

* initialize exchange call on send page load
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
godcr board
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants