-
Notifications
You must be signed in to change notification settings - Fork 25.7k
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
feat(common): add injection token for default DatePipe
configuration
#47157
Conversation
7a63913
to
7342bc8
Compare
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.
@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.
@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! |
8e6740d
to
e6fe1ae
Compare
e6fe1ae
to
d3c8803
Compare
01bfd02
to
09fb9a7
Compare
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.
@matthiasweiss thanks for additional updates, I've left a few more comments. Thank you.
09fb9a7
to
90ef52d
Compare
53679b0
to
f60fa78
Compare
You can preview f60fa78 at https://pr47157-f60fa78.ngbuilds.io/. |
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.
@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.
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.
f60fa78
to
9269a54
Compare
@AndrewKushnir should be ready now! Looking forward to the feedback |
You can preview 9269a54 at https://pr47157-9269a54.ngbuilds.io/. |
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
Reviewed-for: public-api
Reviewed-for: fw-common
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.
reviewed-for: public-api, size-tracking
This PR was merged into the repository by commit bdb5371. |
@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! |
@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 |
@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! |
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. |
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?
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?
Other information
Since there exist no tests for the
DATE_PIPE_DEFAULT_TIMEZONE
injection token, which behaves alomst identically to theDATE_PIPE_DEFAULT_FORMAT
, there are also no tests forDATE_PIPE_DEFAULT_FORMAT
.