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 luxon date adapter #1709

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

angelaki
Copy link
Contributor

I'm switching my project from moment to luxon so I needed luxon support in your (great!) calendar control.

I'm not very familiar with package building but hope I've adopted all the necessary files for a correct luxon support. By the way, since your CalendarNativeDateFormatter uses the Intl object (and I guess for that reason there is no CalendarDateFnsDateFormatter), why is there even a CalendarMomentDateFormatter? Any reason to use it at all?

Thank you for checking, probably some fixing ;) & deploying! :)

@angelaki
Copy link
Contributor Author

Guess it sure doesn't work, yet. But what does that line in the old moment dateAdapter even do?

Just copied it - but actually don't get it.

Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

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

This is really great work, thank you!

Can you also add an example, you can copy from the moment one:

https://github.com/mattlewis92/angular-calendar/tree/main/projects/demos/app/demo-modules/moment

@@ -113,7 +115,7 @@ function addModuleToImports(options: Schema): Rule {
const moduleName = `CalendarModule.forRoot({ provide: DateAdapter, useFactory: ${
options.dateAdapter === 'moment'
? 'momentAdapterFactory'
: 'adapterFactory'
: (options.dateAdapter === 'luxon' ? 'luxonAdapterFactory' : 'adapterFactory')
Copy link
Owner

Choose a reason for hiding this comment

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

Let's bake this into the dateAdapters object and avoid the branching logic e.g.

const dateAdapters: Record<Schema['dateAdapter'], {version: string, adapterName: string}> = {
      moment: {
        version: momentVersion,
        adapterName: 'momentAdapterFactory'
      },
      'date-fns': {
        version: dateFnsVersion,
        adapterName: 'adapterFactory'
      },
      luxon: {
				version: luxonVersion,
				adapterName: 'luxonAdapterFactory'
			},
    };

Then here we can just pass dateAdapters[options.dateAdapter].adapterName;

@@ -156,6 +158,26 @@ function addModuleToImports(options: Schema): Rule {
);
}

if (options.dateAdapter === 'luxon') {
Copy link
Owner

Choose a reason for hiding this comment

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

@@ -0,0 +1,55 @@
import { adapterFactory as baseAdapterFactory } from 'calendar-utils/date-adapters/luxon';
Copy link
Owner

Choose a reason for hiding this comment

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

@mattlewis92
Copy link
Owner

By the way, since your CalendarNativeDateFormatter uses the Intl object (and I guess for that reason there is no CalendarDateFnsDateFormatter), why is there even a CalendarMomentDateFormatter? Any reason to use it at all?

I can't remember the exact reasoning, but I think at the time the Intl API was really buggy in most browsers (which was why angular moved away from it), so I added moment as a "stable" alternative. I think nowadays angular are looking to move back to the Intl api so I guess it got better and having the other date formatters makes less sense.

@mattlewis92 mattlewis92 changed the title Support for Luxon feat: add luxon date adapter Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants