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

[Twig] Currency and Money helpers clean up #4798

Merged
merged 5 commits into from
Apr 20, 2016

Conversation

lchrusciel
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Related tickets
License MIT

@pamil
Copy link
Contributor

pamil commented Apr 15, 2016

Wow, Travis feedback is as fast as it has never been before! 🎉

@pjedrzejewski pjedrzejewski added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Apr 15, 2016
@pjedrzejewski
Copy link
Member

AmountFormatter vs. MoneyFormatter? Amount is quite wide term.

$this->shouldImplement(MoneyHelperInterface::class);
}

function it_allows_to_format_money_in_different_currencies(
Copy link
Contributor

@pamil pamil Apr 15, 2016

Choose a reason for hiding this comment

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

Shouldn't it be divided into more sections:

  • it formats money using default currency and locale if only amount is given
  • it formats money using default currency if not given
  • it formats money using default locale if not given
  • it formats money using given currency and locale

@pamil
Copy link
Contributor

pamil commented Apr 15, 2016

@pjedrzejewski 👍 for MoneyFormatter

@lchrusciel lchrusciel force-pushed the currency-clean-up branch 2 times, most recently from 4c11661 to 723240c Compare April 18, 2016 11:52
@michalmarcinkowski
Copy link
Contributor

👍

@pjedrzejewski pjedrzejewski merged commit 693ade9 into Sylius:master Apr 20, 2016
@pjedrzejewski
Copy link
Member

Thanks Łukasz, really nice work!

@lchrusciel lchrusciel deleted the currency-clean-up branch April 21, 2016 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants