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

feat(material-date-fns-adapter): add date adapter for date-fns #23262

Merged
merged 3 commits into from
Jul 30, 2021
Merged

feat(material-date-fns-adapter): add date adapter for date-fns #23262

merged 3 commits into from
Jul 30, 2021

Conversation

crisbeto
Copy link
Member

Adds a new date adapter that supports the date-fns library.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 28, 2021
@crisbeto crisbeto marked this pull request as ready for review July 28, 2021 20:08
@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release labels Jul 28, 2021
mmalerba
mmalerba approved these changes Jul 28, 2021
View reviewed changes
src/material/core/datetime/date-adapter.ts
@@ -10,20 +10,20 @@ import {inject, InjectionToken, LOCALE_ID} from '@angular/core';
import {Observable, Subject} from 'rxjs';

/** InjectionToken for datepicker that can be used to override default locale code. */
export const MAT_DATE_LOCALE = new InjectionToken<string>('MAT_DATE_LOCALE', {
export const MAT_DATE_LOCALE = new InjectionToken<{}>('MAT_DATE_LOCALE', {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to not just use unknown?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that might be a breaking change, I went with {} since it's what we discussed on Monday.

@crisbeto
feat(material-date-fns-adapter): add date adapter for date-fns
75bba63 
Adds a new date adapter that supports the `date-fns` library.
jelbourn
jelbourn reviewed Jul 28, 2021
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Overall looks good, couple of minor comments

src/material-date-fns-adapter/adapter/date-fns-adapter.ts Show resolved Hide resolved
src/material-date-fns-adapter/adapter/date-fns-adapter.ts Outdated Show resolved Hide resolved
@google-cla

This comment has been minimized.

Sign in to view
@google-cla google-cla bot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jul 29, 2021
@google-cla google-cla bot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jul 29, 2021
@crisbeto @devversion
fixup! feat(material-date-fns-adapter): add date adapter for date-fns
a1c2bfc 
Fix saucelabs unit tests and the integration test for partial
compilation.
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Jul 29, 2021
devversion
devversion approved these changes Jul 29, 2021
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM on the tooling changes.

@zarend
Copy link
Contributor

zarend commented Jul 30, 2021

presubmit (internal). Tests ran last night with 117 failures. A few look like flakes, and many of them are timeouts.

@zarend zarend added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jul 30, 2021
@crisbeto
Copy link
Member Author

Could we re-run it without spending more time investigating it? This is an entirely new package so I doubt that anything in Google would be broken by it. There is a small change to a type in material/core, but it's backwards-compatible.

@zarend
Copy link
Contributor

zarend commented Jul 30, 2021

kk, I kicked off a re-run 🤞

@zarend zarend removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Jul 30, 2021
@zarend zarend merged commit e8dd070 into angular:master Jul 30, 2021
@princemaple
Copy link
Contributor

Hi, curious about what advantages this adapter has.

date-fns operates directly on native date object. I do have date-fns installed and used in my project. Does this adapter improve code size comparing to native adapter?

@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 Sep 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@devversion devversion devversion approved these changes

@mmalerba mmalerba mmalerba approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@crisbeto @zarend @princemaple @jelbourn @devversion @mmalerba