danielmoncada / date-time-picker

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

Angular V13 support. #133

Closed Mekacher-Anis closed 2 years ago

Mekacher-Anis commented 2 years ago

Angular v13 has been released this week. Any plans to publish a version that works with v13 ? I suppose since the angular team is pushing to dropping View Engine, I think this would be a good time to switch to using native Ivy (no ngcc). See this issue https://github.com/angular/angular-cli/issues/16980 . I don't have any experience developing or building angular libraries, but I'm willing to get into it and switch this library to using the Ivy rendering engine. Any Docs or guidelines will be appreciated.

renemadsen commented 2 years ago

Any changes to this?

danielmoncada commented 2 years ago

not yet.. I'll work on getting it out today.

edit: @renemadsen I see that you forked this to add 13 support. would you mind creating a PR for this?

renemadsen commented 2 years ago

@danielmoncada the fork was just an attempt to see, what is missing for updating to angular 13. But since I don't really know what the code is trying to do here, it's difficult to fix it: https://github.com/microting/date-time-picker/runs/4169108246?check_suite_focus=true#step:6:15

    private watchStateChanges(): void {
        this.stateChanges.unsubscribe();

        const inputDisabled = this.dtPicker && this.dtPicker.dtInput ?
            this.dtPicker.dtInput.disabledChange : observableOf();

        const pickerDisabled = this.dtPicker ?
            this.dtPicker.disabledChange : observableOf();

        this.stateChanges = merge(pickerDisabled, inputDisabled)
            .subscribe(() => {
                this.changeDetector.markForCheck();
            });
    }
Argument of type 'Observable<never> | EventEmitter<boolean>' is not assignable to parameter of type 'number'.

If you can tell what is the solution for this?

At first glance, the above error, is the only thing stopping the code from working with angular 13, so far at least :-)

marleypowell commented 2 years ago

@danielmoncada the fork was just an attempt to see, what is missing for updating to angular 13. But since I don't really know what the code is trying to do here, it's difficult to fix it: https://github.com/microting/date-time-picker/runs/4169108246?check_suite_focus=true#step:6:15

    private watchStateChanges(): void {
        this.stateChanges.unsubscribe();

        const inputDisabled = this.dtPicker && this.dtPicker.dtInput ?
            this.dtPicker.dtInput.disabledChange : observableOf();

        const pickerDisabled = this.dtPicker ?
            this.dtPicker.disabledChange : observableOf();

        this.stateChanges = merge(pickerDisabled, inputDisabled)
            .subscribe(() => {
                this.changeDetector.markForCheck();
            });
    }
Argument of type 'Observable<never> | EventEmitter<boolean>' is not assignable to parameter of type 'number'.

If you can tell what is the solution for this?

At first glance, the above error, is the only thing stopping the code from working with angular 13, so far at least :-)

Does it work if you use a different overload of the merge operator? Wrap the observables in an array:

this.stateChanges = merge([pickerDisabled, inputDisabled]).subscribe(() => {
  this.changeDetector.markForCheck();
});
renemadsen commented 2 years ago

Now it can build, but the tests are blowing up.

I don't know karma or how these tests are written, so I don't know how to debug them. https://github.com/microting/date-time-picker/runs/4193651309?check_suite_focus=true#step:7:1

Any suggestions @danielmoncada ?

harvanchik commented 2 years ago

Any update on Angular 13 support?

danielmoncada commented 2 years ago

Updated via https://github.com/danielmoncada/date-time-picker/pull/137

Can you guys @Mekacher-Anis @renemadsen @marleypowell @harvanchik make sure it works on your project before I take it out of alpha? https://www.npmjs.com/package/@danielmoncada/angular-datetime-picker/v/13.0.0-alpha.0

danielmoncada commented 2 years ago

Now it can build, but the tests are blowing up.

I don't know karma or how these tests are written, so I don't know how to debug them. https://github.com/microting/date-time-picker/runs/4193651309?check_suite_focus=true#step:7:1

Any suggestions @danielmoncada ?

Your error indicates it cant find the coverage reporter when it runs.. a bit weird, as I set up the same workflow here and tests are running fine: https://github.com/danielmoncada/date-time-picker/actions

marleypowell commented 2 years ago

Updated via #137

Can you guys @Mekacher-Anis @renemadsen @marleypowell @harvanchik make sure it works on your project before I take it out of alpha? https://www.npmjs.com/package/@danielmoncada/angular-datetime-picker/v/13.0.0-alpha.0

Seems to be working for me with no errors. Nice work 🙂

renemadsen commented 2 years ago

@danielmoncada seems to work. And now I see the difference, I changed the package.json in the root project and not inside the projects folder.

That is what making my repository break.

But why do we have it like that?

danielmoncada commented 2 years ago

@danielmoncada seems to work. And now I see the difference, I changed the package.json in the root project and not inside the projects folder.

That is what making my repository break.

But why do we have it like that?

this is how it was done by the original author. it's separated. the root project is for the "test" app that's coupled with the date/time picker library. any dependency changes on the library itself has to be done with the package.json in the 'projects' folder.

renemadsen commented 2 years ago

this is how it was done by the original author. it's separated. the root project is for the "test" app that's coupled with the date/time picker library. any dependency changes on the library itself has to be done with the package.json in the 'projects' folder.

Makes sense.

I'll try to upgrade it to angular 13, so the project is not so far behind.

danielmoncada commented 2 years ago

closing issue, as this was geared towards ng 13 support. since it now works, I'll create a new issue to upgrade it to ng 13.