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

Use price instead of price_tag #34

Merged
merged 2 commits into from
May 30, 2016
Merged

Use price instead of price_tag #34

merged 2 commits into from
May 30, 2016

Conversation

x4d3
Copy link
Contributor

@x4d3 x4d3 commented May 26, 2016

I think price should be send as a hash instead of a formatted string (because some information is lost, for example $12 can be 12 USD or 12 AUD)
See PR: https://github.com/maestrano/Maestrano/pull/1594

@x4d3
Copy link
Contributor Author

x4d3 commented May 26, 2016

@ouranos Could you please code review ?

# Format a money object
def money(m)
"#{m.format(symbol: false)} #{m.currency_as_string}"
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just m.format? Is it to have the currency after the amount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

m.format is giving something like $ 8, that is ambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah got it, currency_as_string will give USD / AUD

@ouranos
Copy link
Contributor

ouranos commented May 26, 2016

Happy with this as long as @alachaum is happy with maestrano/Maestrano#1594 :)

@x4d3 x4d3 merged commit 2f05f01 into maestrano:master May 30, 2016
aluqueGH pushed a commit to aluqueGH/mno-enterprise that referenced this pull request Jul 10, 2018
[IMPAC-308][WES-29] Fix impac widget loading issues
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.

2 participants