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(datepicker): Create a new injection token to avoid overriding LOCALE_ID #6708

Merged
merged 4 commits into from
Sep 1, 2017

Conversation

g1shin
Copy link

@g1shin g1shin commented Aug 29, 2017

New injection token MAT_DATE_LOCALE is added to allow overriding locale code, while avoiding overriding LOCALE_ID.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 29, 2017
@@ -334,13 +333,13 @@ describe('NativeDateAdapter', () => {
});


describe('NativeDateAdapter with LOCALE_ID override', () => {
describe('NativeDateAdapter with MAT_DATE_LOCALE override', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also have tests for LOCAL_ID override? this should still cascade to MAT_DATE_LOCALE

providers: [{provide: DateAdapter, useClass: NativeDateAdapter}],
providers: [
{provide: DateAdapter, useClass: NativeDateAdapter},
{provide: MAT_DATE_LOCALE, useExisting: LOCALE_ID}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should go in the MdDatepickerModule

Copy link
Contributor

@mmalerba mmalerba Aug 30, 2017

Choose a reason for hiding this comment

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

@jelbourn can you think of a good way to avoid putting this in NativeDateModule, MomentDateModule, etc? I was thinking maybe we could put it in MdCommonModule and have this module import that?

Copy link
Member

Choose a reason for hiding this comment

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

I think it doesn't really belong in MdCommonModule since it is only used in a few components. You could create a separate MdCommonDateModule, but that also seems like overkill. Easier thing would probably be just to make a common provider, e.g.

export const MAT_DATE_LOCALE_PROVIDER = {provide: MAT_DATE_LOCALE, useExisting: LOCALE_ID};

Adding that is about the same as importing another module.

@@ -48,13 +47,15 @@ function range<T>(length: number, valueFunction: (index: number) => T): T[] {
return valuesArray;
}

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

Choose a reason for hiding this comment

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

I would just make the string value of the token the same as the name 'MAT_DATE_LOCALE'


/** Adapts the native JS Date for use with cdk-based components that work with dates. */
@Injectable()
export class NativeDateAdapter extends DateAdapter<Date> {
constructor(@Optional() @Inject(LOCALE_ID) localeId: any) {
constructor(@Optional() @Inject(MAT_DATE_LOCALE) matDateLocale: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any --> string

@g1shin
Copy link
Author

g1shin commented Aug 29, 2017

Comments addressed. Ready for review again 👍

By default the datepicker will use the locale code from the `LOCALE_ID` injection token from
`@angular/core`. If you want to override it, you can provide a new value for the token:

By default `MAT_DATE_LOCALE` injection token will use the existing `LOCALE_ID` locale code from
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: By default MAT_DATE_LOCALE injection token -> By default, the MAT_DATE_LOCALE injection token

@@ -48,13 +47,15 @@ function range<T>(length: number, valueFunction: (index: number) => T): T[] {
return valuesArray;
}

/** InjectionToken for datepicker that can be used to override default locale code. */
export const MAT_DATE_LOCALE = new InjectionToken<string>('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.

can we move this to DateAdapter since we expect all adapters to use it not just NativeDateAdapter?

@g1shin
Copy link
Author

g1shin commented Aug 30, 2017

Changes have been made 😃 Ready for review

@mmalerba
Copy link
Contributor

LGTM

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@kara kara added the action: merge The PR is ready for merge by the caretaker label Aug 31, 2017
@kara kara removed their assignment Aug 31, 2017
@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Aug 31, 2017
@g1shin
Copy link
Author

g1shin commented Aug 31, 2017

Added MAT_DATE_LOCALE_PROVIDER.

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Aug 31, 2017
@jelbourn jelbourn merged commit 2635cad into angular:master Sep 1, 2017
@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 7, 2019
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 cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants