-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat: add luxon date adapter #23167
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 luxon date adapter #23167
Conversation
1765836
to
16e915a
Compare
@crisbeto: as the maintainer of the fork of your original PR, I'd like to notify you that there have been a few fixes over time to it. I don't remember the details for all of them, but take a look at: andreialecu/ngx-material-luxon#4 Also this change is important: See this report: andreialecu/ngx-material-luxon#3 |
The feedback has been addressed. Also thanks for the pointers @andreialecu, I've incorporated most of them. I decided to skip the one about the first day of the week, because I think that we should have a consistent way of doing it with the other adapters. It's also fairly easy to work around by extending the existing adapter and overriding the one method. |
|
||
getDateNames(): string[] { | ||
// At the time of writing, Luxon doesn't offer similar | ||
// functionality so we have to fall back to the Intl API. |
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.
Is this the corresponding github issue?
moment/luxon#549
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.
I don't think so. This isn't really a bug, it's just something that they haven't exposed.
return (this._useUTC ? DateTime.utc() : DateTime.local()).setLocale(this.locale); | ||
} | ||
|
||
parse(value: any, parseFormat: string | string[]): DateTime | null { |
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.
Should we tighten the type of value
to what we actually support in the function? Or potentially even unknown
since we're checking the type anyway.
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.
It's an any
, because the DateAdapter
interface has it as an any
too.
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.
Yeah, I think it makes sense for the abstract adapter to be any
, but IIRC subclasses are able to narrow signatures, right? (Non-blocking)
Adds a new package called `@angular/material-luxon-adapter` that provides a date adapter to be used together with Luxon dates.
The latest feedback has been addressed. |
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
Caretaker note from Jeremy:
|
I saw that this was merged and that releases happened since, however the I'm only mentioning it in case this isn't a know issue, and something related to build / release setup was overlooked. |
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. |
Adds a new package called
@angular/material-luxon-adapter
that provides a date adapter to be used together with Luxon dates.