Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add currency symbols where appropriate #6249

Merged
merged 1 commit into from
Dec 17, 2016
Merged

Conversation

jkup
Copy link
Contributor

@jkup jkup commented Dec 16, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Fix #3533

Auditors @bsclifton @cezaraugusto

Test Plan:

  1. Go to Preferences > Payments
  2. Check the payment amount, the current balance, and your payment history and make sure they all have $ for currency.

screen shot 2016-12-16 at 1 25 07 am

@bsclifton
Copy link
Member

How is this.props.ledgerData.get('currency') determined? I'm guessing it's based on the IP?

@jkup can you screenshot the places where currency is updated? I'm also curious how the formatting looks for other currency

@jkup
Copy link
Contributor Author

jkup commented Dec 16, 2016

@bsclifton yep it's based on the IP. I added a screenshot of what this looks like for USD but I'm a little hesitant to ship this ticket because after talking with @mrose17 it turns out faking the currency is difficult to do.

I suppose my only real worry is that we support some currency that doesn't exist in this package's currency map (https://github.com/bengourley/currency-symbol-map/blob/master/map.js) so it will default to USD instead. Thoughts?

@mrose17
Copy link
Member

mrose17 commented Dec 16, 2016

the list is fairly comprehensive, so i'm no worried about it.

we do not have plans to turn on additional currencies right now, so i don't think this is a priority.

i would do the PR and then mark the issue closed.

@cezaraugusto
Copy link
Contributor

++ lgtm

@bsclifton
Copy link
Member

Thanks for the update! Looks good 😄

@cezaraugusto cezaraugusto merged commit 4f5a4f3 into master Dec 17, 2016
@luixxiul
Copy link
Contributor

Note: npm i is required to fix the error

Uncaught TypeError: getSymbolFromCurrency is not a function{TypeError: getSymbolFromCurrency is not a function

@jkup
Copy link
Contributor Author

jkup commented Dec 17, 2016

@luixxiul yep this is how adding npm packages works

@bradleyrichter
Copy link
Contributor

@cezaraugusto This PR is useful but needs to be corrected ASAP to show the ISO 4217 currency code (3 letters) instead of the symbol.

@bsclifton
Copy link
Member

This was reverted; removing milestone

@bsclifton bsclifton removed this from the 0.13.0 milestone Jan 19, 2017
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.

7 participants