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.

Sign up for GitHub

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

Jump to bottom

fix(common): format day-periods that cross midnight #36611

Closed
wants to merge 1 commit into from
Closed

fix(common): format day-periods that cross midnight #36611

wants to merge 1 commit into from

Conversation

petebacondarwin
Copy link
Member

When formatting a time with the b or B format codes,
the resulting string should represent a period of the day.

The logic for computing this period is based on start and end
times, but the code was not able to cope with a period that
crossed midnight.

Fixes #36566

@petebacondarwin petebacondarwin added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package area: i18n labels Apr 13, 2020
@ngbot ngbot bot added this to the needsTriage milestone Apr 13, 2020
@pullapprove pullapprove bot requested a review from mhevery April 13, 2020 20:07
@petebacondarwin petebacondarwin requested review from AndrewKushnir and removed request for mhevery April 13, 2020 20:07
@petebacondarwin petebacondarwin added the target: patch This PR is targeted for the next patch release label Apr 13, 2020
AndrewKushnir
AndrewKushnir approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir 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 the fix @petebacondarwin! 👍

I just left a couple minor comments, please have a look when you get a chance.

Quick question: should we consider this fix as a (minor) breaking change? Even though it's a bug fix, the behavior is slightly changed, so code that relied on the old behavior might need to be updated. What do you think @petebacondarwin?

packages/common/src/i18n/format_date.ts Show resolved Hide resolved
packages/common/test/i18n/format_date_spec.ts Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 13, 2020
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Apr 13, 2020 •
edited

I've started Blueprint Presubmit and Global Presubmit. Thank you.

@petebacondarwin
Copy link
Member Author

Personally, I don't see this as a breaking change. Sure the behaviour changed - but that is because it was broken before, no? I will change the commit message to highlight this change to what is output.

@petebacondarwin
Copy link
Member Author

I added a comment and updated the commit message - no code changes, so the presubmits should stand.

@AndrewKushnir AndrewKushnir removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit labels Apr 14, 2020
@AndrewKushnir
Copy link
Contributor

@petebacondarwin FYI, Blueprint and Global presubmits are successful 👍

@petebacondarwin
fix(common): format day-periods that cross midnight
8be5779 
When formatting a time with the `b` or `B` format codes, the rendered
string was not correctly handling day periods that spanned midnight.
Instead the logic was falling back to the default case of `AM`.

Now the logic has been updated so that it matches times that are within
a day period that spans midnight, so it will now render the correct
output, such as `at night` in the case of English.

Applications that are using either `formatDate()` or `DatePipe` and any
of the `b` or `B` format codes will be affected by this change.

Fixes angular#36566
@petebacondarwin petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Apr 15, 2020
@petebacondarwin
Copy link
Member Author

@AndrewKushnir do we need to run presubmit again, or can we add merge-assistance to push this through?

@AndrewKushnir
Copy link
Contributor

@petebacondarwin, I think it's safe to reuse existing g3 global presubmit, so I forced "green" g3 status for this PR. Thank you.

@atscott atscott closed this in c6e5fc4 Apr 16, 2020
atscott pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @atscott
fix(common): format day-periods that cross midnight (#36611)
1756cce 
When formatting a time with the `b` or `B` format codes, the rendered
string was not correctly handling day periods that spanned midnight.
Instead the logic was falling back to the default case of `AM`.

Now the logic has been updated so that it matches times that are within
a day period that spans midnight, so it will now render the correct
output, such as `at night` in the case of English.

Applications that are using either `formatDate()` or `DatePipe` and any
of the `b` or `B` format codes will be affected by this change.

Fixes #36566

PR Close #36611
@petebacondarwin petebacondarwin deleted the i18n-format-B branch April 16, 2020 16:46
matsko added a commit to matsko/angular that referenced this pull request Apr 22, 2020
@matsko
revert: fix(common): format day-periods that cross midnight (angular#…
7b28667 
…36611)

This reverts commit 1756cce.
matsko added a commit to matsko/angular that referenced this pull request Apr 22, 2020
@matsko
revert: fix(common): format day-periods that cross midnight (angular#…
039f37d 
…36611)

This reverts commit 1756cce.

The reason for this revert is because of the change being too risky for
a patch release of Angular. Changing date formatting proves to be a
breaking change and can only be apart of the next release.
matsko added a commit that referenced this pull request Apr 22, 2020
@matsko
revert: fix(common): format day-periods that cross midnight (#36611) (#…
8bd5374 
…36751)

This reverts commit 1756cce.

The reason for this revert is because of the change being too risky for
a patch release of Angular. Changing date formatting proves to be a
breaking change and can only be apart of the next release.

PR Close #36751
@IgorMinar IgorMinar mentioned this pull request May 16, 2020
Closed
3 tasks
@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 May 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package area: i18n cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

formatDate returns AM/PM for "B" pattern in locale "en"
3 participants
@petebacondarwin @AndrewKushnir @googlebot