-
Notifications
You must be signed in to change notification settings - Fork 25k
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
fix(common): fix currencyCode in CurrencyPipe #40505
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR - this fix looks sound.
Could you please provide a unit test for this functionality by adding an it
clause to the describe block at
describe('CurrencyPipe', () => { |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gvmohzibat - we are getting there. Just a couple more tweaks.
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
Global Presubmit (internal-only link). |
Quick update after running tests in Google's codebase: this change is causing some internal tests to fail and more investigation is needed, so I'm adding the "blocked" label for now. We'll perform further investigation and keep this thread updated. Thank you. |
I'm not sure if I correctly rebased it. But I think it's good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🍪
currencyCode should be initialized with the injected default currency code
currencyCode should be initialized with the injected default currency code PR Close #40505
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When providing
DEFAULT_CURRENCY_CODE
, it's expected that theCurrenyPipe
would pass it ascurrencyCode
toformatCurrency
function.PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently providing the
DEFAULT_CURRENCY_CODE
changes the symbol that's being displayed but it's not used inside theformatCurrency
function for getting theminFrac
(link to source code), therefore the default value (USD) is being used. So as an example, despite providing{ provide: DEFAULT_CURRENCY_CODE, useValue: 'IRR' }
, the code{{ 1000 | currency }}
will result in1,000.00
but it should be1,000
based on this lineWhat is the new behavior?
With this change, the provided value for
DEFAULT_CURRENCY_CODE
will be passed ascurrencyCode
toformatCurrency
function.Does this PR introduce a breaking change?
Other information