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

fix(common): remove trailing whitespace for CurrencyPipe #34642

Conversation

troelslenda
Copy link
Contributor

@troelslenda troelslenda commented Jan 5, 2020

Trimming any surrounding whitespace characters in
formatNumberToLocaleString if currency symbol is
supressed.

Closes #34641

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  •  Bugfix
  •  Feature
  •  Code style update (formatting, local variables)
  •  Refactoring (no functional changes, no api changes)
  •  Build related changes
  •  CI related changes
  •  Documentation content changes
  •  angular.io application / infrastructure changes
  •  Other... Please describe:

What is the current behavior?

Issue Number: #34641

What is the new behavior?

The exessive whitespace is trimmed.

Does this PR introduce a breaking change?

  •  Yes
  •  No

@troelslenda troelslenda requested review from a team as code owners January 5, 2020 21:11
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @troelslenda - the main fix and the tests look great. Could you remove the additional refactorings that were added (i.e. the addition and removal of parentheses, and the reformatting) since they are not relevant to this fix.

@petebacondarwin petebacondarwin added target: patch This PR is targeted for the next patch release type: bug/fix area: i18n action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 6, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jan 6, 2020
@troelslenda troelslenda force-pushed the remove-currency-pipe-trailing-space branch from 5812329 to f051afd Compare January 6, 2020 13:54
@troelslenda
Copy link
Contributor Author

Hi @petebacondarwin!

Thank you for your quick response. I've cleaned up the PR with a git commit --amend.

@petebacondarwin
Copy link
Member

Great! Thanks.
Unfortunately, your amended commit message is all on one line, which is breaking the lint checks.

Trimming any surrounding whitespace characters informatNumberToLocaleString
if currency symbol issupressed.

Closes angular#34641
@troelslenda troelslenda force-pushed the remove-currency-pipe-trailing-space branch from f051afd to fb184e9 Compare January 6, 2020 14:02
@troelslenda
Copy link
Contributor Author

Hi @petebacondarwin
It should be good now. No failing tests it seems

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jan 6, 2020
@AndrewKushnir
Copy link
Contributor

Presubmit

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jan 7, 2020
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 7, 2020
alxhub pushed a commit that referenced this pull request Jan 7, 2020
Trimming any surrounding whitespace characters informatNumberToLocaleString
if currency symbol issupressed.

Closes #34641

PR Close #34642
@alxhub alxhub closed this in 1b6ad15 Jan 7, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: i18n cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excessive whitespace character in CurrencyPipe
4 participants