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

feat(common): add injection token for default DatePipe configuration #47157

Closed

Conversation

matthiasweiss
Copy link
Contributor

@matthiasweiss matthiasweiss commented Aug 15, 2022

In order to globally change the default format of the Angular date pipe, a new injection token called "DATE_PIPE_DEFAULT_FORMAT" is added and used in the date pipe

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?

Currently the date pipe defaults to the mediumDate format and there is no way to override this behavior with an injection token. If it is not present, the behavior is identical.

What is the new behavior?

The DATE_PIPE_DEFAULT_FORMAT injection token can be used to override the default format that the Angular date pipe uses.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Since there exist no tests for the DATE_PIPE_DEFAULT_TIMEZONE injection token, which behaves alomst identically to the DATE_PIPE_DEFAULT_FORMAT, there are also no tests for DATE_PIPE_DEFAULT_FORMAT.

@jessicajaniuk jessicajaniuk added the area: common Issues related to APIs in the @angular/common package label Aug 15, 2022
@ngbot ngbot bot added this to the Backlog milestone Aug 15, 2022
@jessicajaniuk jessicajaniuk added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release labels Aug 15, 2022
@matthiasweiss matthiasweiss force-pushed the add_default_format_date_pipe branch 3 times, most recently from 7a63913 to 7342bc8 Compare August 15, 2022 18:34
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.

@matthiasweiss thanks for the PR. It looks like this feature was requested in #39194, but the ticket was closed with the proposal to add a custom pipe based on DatePipe.

However, since we've added the DATE_PIPE_DEFAULT_TIMEZONE later in #43611, I think adding a token to configure date format makes sense and makes the DatePipe API more consistent.

We'll discuss it further with the team and I'll post an update.

Since there exist no tests for the DATE_PIPE_DEFAULT_TIMEZONE injection token, which behaves alomst identically to the DATE_PIPE_DEFAULT_FORMAT, there are also no tests for DATE_PIPE_DEFAULT_FORMAT.

PR #43611 (where the DATE_PIPE_DEFAULT_TIMEZONE was implemented) has some relevant tests. Could you please add similar ones? I think it'd be also great to have a test where a component is created via TestBed and uses DatePipe in a template, so that the DATE_PIPE_DEFAULT_FORMAT token behavior is tested additionally.

@matthiasweiss
Copy link
Contributor Author

@AndrewKushnir thanks for the note on the existing tests, I just globally searched for the token and there was nothing there so I thought there are none, my bad. I'll try to add tests sometime this week, should not take too long anyway!

@matthiasweiss matthiasweiss force-pushed the add_default_format_date_pipe branch 2 times, most recently from 8e6740d to e6fe1ae Compare September 5, 2022 15:53
@matthiasweiss matthiasweiss requested review from AndrewKushnir and removed request for alxhub September 5, 2022 15:55
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.

@matthiasweiss thanks for additional updates, I've left a few more comments. Thank you.

packages/common/src/pipes/date_pipe.ts Outdated Show resolved Hide resolved
packages/common/src/pipes/date_pipe_config.ts Show resolved Hide resolved
packages/common/src/pipes/index.ts Show resolved Hide resolved
packages/common/src/pipes/index.ts Show resolved Hide resolved
packages/common/src/pipes/date_pipe.ts Outdated Show resolved Hide resolved
@mary-poppins
Copy link

You can preview f60fa78 at https://pr47157-f60fa78.ngbuilds.io/.

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.

@matthiasweiss thanks again for all the changes 👍

I've just left a minor comment, but overall the change looks good.

The next steps for this change:

  • could you please address that small comment?
  • we'll need a couple more approvals from the public-api group (there might be additional feedback/comments)
  • we'll run the necessary tests in Google's codebase and let you know the result

Thank you.

packages/common/src/pipes/date_pipe.ts Outdated Show resolved Hide resolved
This commit introduces a new `DATE_PIPE_DEFAULT_OPTIONS` token, which
can be used to configure default DatePipe options, such as date
format and timezone.

DEPRECATED:

The `DATE_PIPE_DEFAULT_TIMEZONE` token is now deprecated in favor
of the `DATE_PIPE_DEFAULT_OPTIONS` token, which accepts an object
as a value and the timezone can be defined as a field (called `timezone`)
on that object.
@matthiasweiss
Copy link
Contributor Author

@AndrewKushnir should be ready now! Looking forward to the feedback

@mary-poppins
Copy link

You can preview 9269a54 at https://pr47157-9269a54.ngbuilds.io/.

Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api
Reviewed-for: fw-common

@pullapprove pullapprove bot requested a review from atscott October 6, 2022 07:56
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api, size-tracking

@pullapprove pullapprove bot removed the request for review from atscott October 7, 2022 16:13
@jessicajaniuk jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 7, 2022
@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit bdb5371.

@AndrewKushnir
Copy link
Contributor

@matthiasweiss thanks again for all your help with this feature 👍

The PR was merged and this feature will be available to all Angular users as a part of upcoming v15 release 🎉

Thanks for contributing to Angular!

@AndrewKushnir
Copy link
Contributor

@matthiasweiss thanks again for contributing this feature to Angular 👍

We want to mention this feature and yourself as an author in an upcoming blog post (about v15 release). Please let us know if that's ok or you'd prefer not to be mentioned in the blog post.

// cc @mgechev

@matthiasweiss
Copy link
Contributor Author

@AndrewKushnir @mgechev Unfortunately I did not see your comment in time 😢 (a co-worker just showed me now)

If there is a chance that the blog post could be updated, then I'd love to be mentioned!

@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 Dec 18, 2022
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 aio: preview area: common Issues related to APIs in the @angular/common package feature Issue that requests a new feature target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants