danielmoncada / date-time-picker

Angular Date Time Picker (Responsive Design)
https://daniel-projects.firebaseapp.com/owlng/date-time-picker
MIT License
137 stars 59 forks source link

Angular 17 #188

Closed neodescis closed 6 months ago

neodescis commented 8 months ago

It would be great to add support for Angular 17. I've already tested things by overriding the peer dependencies, and everything seems to work just fine, so I think we just need to update the peer dependencies and publish.

I would be happy to create a pull request, but I'm wondering... instead of just adding || ^17.0.0, can we specify >= 13.0.3?

neodescis commented 8 months ago

Also, @danielmoncada, if you're looking for someone to help out on this repo, I'd be happy to take up a contributor or even maintainer role.

polhek commented 8 months ago

@neodescis i was trying to make the library work locally to test if it works. But after "npm install" and "ng serve" in the root directory, I get quite a lot of errors. That is without any changes, just forking the repo and trying it out. Can you point me in the right direction, please.

Node version: v16.20.2

image

danielmoncada commented 8 months ago

@neodescis ah, didn't realize a new ng version was out. yeah, I can try and update it tonight, thanks for testing.

neodescis commented 7 months ago

@neodescis i was trying to make the library work locally to test if it works. But after "npm install" and "ng serve" in the root directory, I get quite a lot of errors. That is without any changes, just forking the repo and trying it out. Can you point me in the right direction, please.

Node version: v16.20.2

image

When I say I got things working with Angular 17, I mean I just added an entry to "overrides" in my project's package.json to get past the peer dependency error with installing and running Angular 17 along side this package. I did not do what you attempted.

I will also add that if we update this package to be built with Angular 17, it will break compatibility with previous Angular versions, as built libraries are not backward compatible with earlier versions of Angular. Rather, my point was that updating the peer dependencies is good enough.

polhek commented 7 months ago

@neodescis i was trying to make the library work locally to test if it works. But after "npm install" and "ng serve" in the root directory, I get quite a lot of errors. That is without any changes, just forking the repo and trying it out. Can you point me in the right direction, please.

Node version: v16.20.2

image

When I say I got things working with Angular 17, I mean I just added an entry to "overrides" in my package.json to get past the peer dependency error with installing and running Angular 17 along side this package. I did not do what you attempted.

Also, if we update this package to be built with Angular 17, it will break compatibility with previous Angular versions, as built libraries are not backward compatible with earlier versions of Angular. Rather, my point was that updating the peer dependencies is good enough.

I forked the repo, then 'npm install' and then ng serve or one of the build the library, but none of it worked. Resulting in the errors in the screenshot.

neodescis commented 7 months ago

I forked the repo, then 'npm install' and then ng serve or one of the build the library, but none of it worked. Resulting in the errors in the screenshot.

Yes, I understand. I ran npm install, npm run build_lib and npm start and things are working just fine for me. I used node 16.19.0 because I already had it installed via nvm.

pavlikxor commented 7 months ago

Hi Everyone @danielmoncada Please check the PR Added Angular 17 support https://github.com/danielmoncada/date-time-picker/pull/189

neodescis commented 7 months ago

@pavlikxor, as I mentioned above, if we switch to actually building with Angular 17, it is no longer backward compatible with earlier versions of Angular. Is that what we want to do?

neodescis commented 7 months ago

Also, this is not the only repository that needs updating. There are separate repos for moment and Day.js adapters.

pavlikxor commented 7 months ago

@pavlikxor, as I mentioned above, if we switch to actually building with Angular 17, it is no longer backward compatible with earlier versions of Angular. Is that what we want to do?

Correct, I see no reasons keeping it backward compatible. For apps developed using older Angular versions (<= 16) appropriate library version should be used

pavlikxor commented 7 months ago

Also, this is not the only repository that needs updating. There are separate repos for moment and Day.js adapters.

Correct, I'll take care of adapter-libraries update as well. But firstly main library needs to be updated, as adapter-libraries have devDependency on @danielmoncada/angular-datetime-picker

NetWin commented 7 months ago

What's the current state here? Is there an ETA for the release? 😄 @danielmoncada @neodescis @pavlikxor

neodescis commented 7 months ago

I believe there are two things outstanding in the code review that @danielmoncada had thumbs-up'd:

  1. The change to disableClose in dialog-ref.class.ts
  2. Changing the peer dependencies to be >= 17.0.0
danielmoncada commented 7 months ago

I have some time now.. pulling the PR now

danielmoncada commented 7 months ago

alright; latest has been published to npm. still need to update the adapters.

pavlikxor commented 7 months ago

Hi everyone cc @danielmoncada Moment adapter, please review - https://github.com/danielmoncada/date-time-picker-moment-adapter/pull/15 Dayjs adapter - https://github.com/danielmoncada/date-time-picker-dayjs-adapter/pull/15

neodescis commented 7 months ago

alright; latest has been published to npm. still need to update the adapters.

I guess we're not going with >=17.0.0 in the peer dependencies then? Bummer.

neodescis commented 7 months ago

Hi everyone cc @danielmoncada Moment adapter, please review - danielmoncada/date-time-picker-moment-adapter#15 Dayjs adapter - danielmoncada/date-time-picker-dayjs-adapter#15

I added a few comments to the dayjs adapter. The moment adapter seems like it should have similar tsconfig changes to what you did for dayjs though, no?

danielmoncada commented 6 months ago

closing, both adapters and the picker have now been updated (and published to npm).

thank you for the help / work @pavlikxor and @neodescis