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: add date adapter for luxon #14681

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

Adds the LuxonDateAdapter that can be used to work with Luxon dates in the MatDatepicker.

@crisbeto crisbeto added the in progress This issue is currently in progress label Dec 30, 2018
@crisbeto crisbeto self-assigned this Dec 30, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 30, 2018
@crisbeto crisbeto force-pushed the luxon-date-adapter branch 5 times, most recently from 06cf8c8 to f917d7f Compare December 30, 2018 21:13
@crisbeto crisbeto changed the title wip: add date adapter for luxon feat: add date adapter for luxon Dec 31, 2018

load("//tools:defaults.bzl", "ng_module", "ng_package")
load("//:packages.bzl", "ROLLUP_GLOBALS")

Copy link
Member Author

Choose a reason for hiding this comment

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

@devversion this file is based on the one from the Moment adapter, but I'm not sure whether it's entirely correct. AFAIK this won't run the unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably update this now, based on #14687

@crisbeto crisbeto assigned jelbourn, devversion and mmalerba and unassigned crisbeto Dec 31, 2018
@crisbeto crisbeto added target: minor This PR is targeted for the next minor release and removed in progress This issue is currently in progress labels Dec 31, 2018

load("//tools:defaults.bzl", "ng_module", "ng_package")
load("//:packages.bzl", "ROLLUP_GLOBALS")

Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably update this now, based on #14687

}

getFirstDayOfWeek(): number {
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe leave a comment about Luxon not supporting this

}

clone(date: DateTime): DateTime {
return date;
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't look like this is actually cloning?

Copy link
Member Author

@crisbeto crisbeto Jan 3, 2019

Choose a reason for hiding this comment

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

We don't need to clone since the Luxon DateTime is immutable. I'll leave a comment.

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 we still ought to, otherwise d === adapter.clone(d) which feels wrong

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that it should still be a copy

@crisbeto
Copy link
Member Author

crisbeto commented Jan 3, 2019

I've addressed the feedback @mmalerba. Also @devversion can you take a look at the Bazel setup? I've based it off of what you did for the Moment adapter.

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 Bazel stuff. Just a few minor nits.

deps = [
"@angular//packages/core",
"@matdeps//@types/luxon",
"@matdeps//luxon",
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this here (only the types). Moment ships its typings as part of @matdeps//moment

# Explicitly specify the tsconfig that is also used by Gulp. We need to explicitly use this
# tsconfig because in order to import Luxon with TypeScript, we need some special options
# enabled.
tsconfig = ":tsconfig-build.json",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe my commenting wasn't too clear, but we shouldn't need this here because Luxon doesn't need the synthetic imports mentioned in the tsconfig-build.json of the moment adapter package.

"//src/lib/testing",
"//src/cdk/platform",
"@matdeps//@types/luxon",
"@matdeps//luxon",
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. Should be able to remove this from here.

}

clone(date: DateTime): DateTime {
return date;
Copy link
Member

Choose a reason for hiding this comment

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

Agreed that it should still be a copy

|---------------------|-----------|-----------------------------------------------------------------------|----------------------------------------|----------------------------------|
|`MatNativeDateModule`|`Date` |en-US |None |`@angular/material` |
|`MatMomentDateModule`|`Moment` |[See project](https://github.com/moment/moment/tree/develop/src/locale)|[Moment.js](https://momentjs.com/) |`@angular/material-moment-adapter`|
|`MatLuxonDateModule` |`DateTime` |[See project](https://moment.github.io/luxon/docs/manual/intl.html) |[Luxon](https://moment.github.io/luxon/)|`@angular/material-luxon-adapter`|

*Please note: `MatNativeDateModule` is based off of the functionality available in JavaScript's
Copy link
Member

Choose a reason for hiding this comment

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

We should update this note to just say "...highly recommend using one of the alternative adapters or creating your own custom adapter"

@ngbot
Copy link

ngbot bot commented Jan 9, 2019

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment
@ngbot
Copy link

ngbot bot commented Jan 9, 2019

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto
Copy link
Member Author

crisbeto commented Jan 9, 2019

@jelbourn @mmalerba I've addressed the feedback. Can you take another look?

Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM, we should do a follow-up PR to add docs for it

Adds the `LuxonDateAdapter` that can be used to work with Luxon dates in the `MatDatepicker`.
}

getDayOfWeekNames(style: 'long' | 'short' | 'narrow'): string[] {
return Info.weekdays(style, {locale: this.locale});

Choose a reason for hiding this comment

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

Not sure if should be changed here or somewhere else, but the way this is, the weekdays on the month view of the calendar don't line up correctly with the dates because it starts on Monday instead of Sunday. For example, April 1, 2019 was on a Monday, but this adapter causes the 'T' for Tuesday to appear above April 1 instead of the 'M'.

return Info.months(style, {locale: this.locale});
}

getDateNames(): string[] {

Choose a reason for hiding this comment

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

this function might be implemented like this:

Suggested change
getDateNames(): string[] {
public getDateNames(): string[] {
return range(31, day =>
new DateTime()
.set({ month: 1, day })
.setLocale(this.locale)
.toFormat('d')
);
}

@LosD
Copy link

LosD commented Jul 23, 2019

Blocked pending further discussion on our strategy for supporting multiple date adapters

@andrewseguin Did you ever complete that discussion?

@dmitrye
Copy link

dmitrye commented Aug 8, 2019

It would be good for someone to finish this adapter. The current method I had to resort to was to make properties for each Date Input

The html looks like this:

<input matInput id="endDate" formControlName="endDate" name="endDate" placeholder="Select a Date" autocomplete="off"
                               [value]="endDateAsDate" type="text" class="form-control" [matDatepicker]="myEndDatePicker" (focus)="myEndDatePicker.open()">

The .ts file looks like this (converts Luxon DateTime to JSDate/Date object):

  get endDateAsDate(){
    return <Date>(<DateTime>this.f.endDate.value).toJSDate();
  }

It's only needed to set the default value coming back from DB or if you want to preset it to Today. But remembering to do this everywhere doesn't seem sustainable.

@Enngage
Copy link

Enngage commented Aug 10, 2019

+ Luxon date adapter

@weihsth
Copy link

weihsth commented Aug 29, 2019

Is the Luxon date adapter still a topic? Would be really cool... it's 5 month now and no progress anymore(?) =)

@DaSchTour
Copy link
Contributor

I think with google maps and youtube player as new modules in this repository there should not be any doubt about adding this module here.

@sbarbat
Copy link

sbarbat commented Nov 23, 2020

Hi guys, what is needed to merge this? Happy to help here 😄

@LosD
Copy link

LosD commented Nov 23, 2020

Hi guys, what is needed to merge this? Happy to help here 😄

@sbarbat unfortunately, it is becoming a standard pattern for this project: Things moving forward nicely, then @andrewseguin coming in with some "we need to discuss this", that discussion apparently never taking place, and everything dying from there on out.

I fully understand the need to design things properly, but if you decide on a discussion, effing HOLD that discussion instead of just letting stuff die for years.

@jelbourn
Copy link
Member

Status us tracked here: #20599 (comment)

The gist is that there's debate on the team as to whether we should support these adapters, or defer to community implementations.

@mtpultz
Copy link

mtpultz commented Dec 15, 2020

@andrewseguin @jelbourn would it not make sense to support at least one new date adapter while you internally decide whether you want to maintain date adapters in the Angular repo, or push these adapters out for the Angular community to maintain? By allowing this PR to continue and be merged in you're not locking Angular into maintaining date adapters.

Supporting Luxon in Angular makes sense since you currently support a Moment date adapter, and Luxon is essentially Moment v2 so all you are doing is providing an update due to a deprecation, which is good practice and a maintenance task. You can still have the discussion and split out the Luxon date adapter into a community repo afterwards, but by having this on hold and opening an internal discussion on whether Angular should or should not support date adapters (that apparently hasn't happened) doesn't stop new projects from perpetuating the use of Moment.

@andrewseguin #14681 (comment) you blocked a PR that was apparently marked ready to merge. With the offers to help from the community and the amount of work already put into this PR this could have been completed and pushed in leaving all sorts of time for an internal discussion that the community isn't privy on where date adapters should live, and it would solve the issue we're having with a deprecated dependency being the only officially supported fluent date adapter in Angular.

@jelbourn #20599 (comment) is an opinion PR that discusses which date library should have a date adapter, but you can't discuss what libraries should have date adapters if internally no decision has been made on whether they should be maintained by Angular or the community.

This PR has support from the community. Please take the hold off to remove the bottleneck on this date adapter so we can adopt a new date adapter, drop a deprecated dependency, and then where everything ends up living can be decided.

@andreialecu
Copy link

I ended up needing this date adapter as well, and just copied the code from this PR and published it on npm: https://www.npmjs.com/package/ngx-material-luxon

Seems to work for my purposes.

@jelbourn
Copy link
Member

I would personally like to move forward with this, but I don't have unilateral decision-making power when it comes to publishing new @angular packages, and unfortunately there isn't consensus from the rest of the Angular team that this is something we should work on yet. Overall the team will be in a better position to consider additions like this once ViewEngine (the pre-Ivy rendering engine) is deleted.

@Polyterative
Copy link

any progress on this?

@wiegell
Copy link

wiegell commented Apr 17, 2021

Any news?

@crisbeto
Copy link
Member Author

Closing in favor of #23167.

@crisbeto crisbeto closed this Jul 14, 2021
@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 Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked This issue is blocked by some external factor, such as a prerequisite PR cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note 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