18779 Use Vuetify 3 DatePicker

bcgov / bcrs-shared-components

BCRS Shared Components is a multi-package Lerna repository of shared Vue components, each published individually and that you can explore/ develop/ document/ test using Storybook.

https://bcgov.github.io/bcrs-shared-components/

Apache License 2.0

3 stars 30 forks source link

18779 Use Vuetify 3 DatePicker #215

Closed leodube-aot closed 10 months ago

leodube-aot commented 10 months ago

Issue #: /bcgov/entity#18779

Description of changes:

Updated Vuetify 3 to version with DatePicker component
Updated our DatePicker component to use the new one from Vuetify 3. The storybook stories seem to all work the same as our previous DatePicker
Updated storybook files
I'll rebase and ~finish up unit tests~ (tech debt ticket created instead) once Ketaki's PR (#212 ) is merged as she has fixed up the unit test setup

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the business-filings-ui license (Apache 2.0).

leodube-aot commented 10 months ago

@severinbeauvais @JazzarKarim @ketaki-deodhar I'm having an issue getting the unit tests to work for this component due to the DateMixin mixin. I'm wondering if any of you have encountered anything similar with mixins in Vue 3 components.

The way original way we had it, which is the recommended way from vue-facing-decorator yields the following error in storybook:

this.yyyyMmDdToPacificDate is not a function

Changing it to mixins: [toNative(DateMixin)] also doesn't work. The way I got it working in storybook for this PR is to extend the DateMixin, but the unit tests fail with a TypeError.

Has anyone gotten unit tests to work for a component that uses a mixin yet? Been stuck on this for a little bit now and don't want to get too bogged down in it.

severinbeauvais commented 10 months ago

@severinbeauvais @JazzarKarim @ketaki-deodhar I'm having an issue getting the unit tests to work for this component due to the DateMixin mixin. I'm wondering if any of you have encountered anything similar with mixins in Vue 3 components.

Does it work if you import the "local" mixin instead of the shared one?

leodube-aot commented 10 months ago

Does it work if you import the "local" mixin instead of the shared one?

No unfortunately it doesn't

severinbeauvais commented 10 months ago

Does it work if you import the "local" mixin instead of the shared one?

No unfortunately it doesn't

OK, that's partly good news. (It means the previous issue was resolved.)

What if you add the shared mixin to Storybook's (ie, this repo's) package file?

leodube-aot commented 10 months ago

What if you add the shared mixin to Storybook's (ie, this repo's) package file?

It looks like the shared mixin is already a dependency of the base package.json

severinbeauvais commented 10 months ago

What if you add the shared mixin to Storybook's (ie, this repo's) package file?

It looks like the shared mixin is already a dependency of the base package.json

Oh yeah. So maybe it's a Storybook build/config issue? How does Storybook build this so that DatePicker can see the mixins?

@ketaki-deodhar , have you run across this yet?

ketaki-deodhar commented 10 months ago

What if you add the shared mixin to Storybook's (ie, this repo's) package file?

It looks like the shared mixin is already a dependency of the base package.json

Oh yeah. So maybe it's a Storybook build/config issue? How does Storybook build this so that DatePicker can see the mixins?

@ketaki-deodhar , have you run across this yet?

@severinbeauvais not really. Let me take a look

ketaki-deodhar commented 10 months ago

@severinbeauvais @JazzarKarim @ketaki-deodhar I'm having an issue getting the unit tests to work for this component due to the DateMixin mixin. I'm wondering if any of you have encountered anything similar with mixins in Vue 3 components.

The way original way we had it, which is the recommended way from vue-facing-decorator yields the following error in storybook:
this.yyyyMmDdToPacificDate is not a function
Changing it to mixins: [toNative(DateMixin)] also doesn't work. The way I got it working in storybook for this PR is to extend the DateMixin, but the unit tests fail with a TypeError.

Has anyone gotten unit tests to work for a component that uses a mixin yet? Been stuck on this for a little bit now and don't want to get too bogged down in it.

can you try passing the mixin in the mount? Not sure if it will work or if right way to do it but something like below.....

leodube-aot commented 10 months ago

can you try passing the mixin in the mount? Not sure if it will work or if right way to do it but something like below..... !

No luck unfortunately. I passed the mixin to the mount in the unit tests like so:

I ran it with the previous method of adding the mixin to the component, which doesn't seem to register the mixin based on the error message. In this case, both storybook and the unit tests fail.

I also tried running it with the way I extend the mixin in this PR. In this case storybook works but the unit tests fail.

severinbeauvais commented 10 months ago

I also tried running it with the way I extend the mixin in this PR. In this case storybook works but the unit tests fail.

OK, you got it down to a unit test error. That's progress. And if we decide to give up, we can forgo the unit tests for now (but we'd create a Tech Debt ticket to fix them later).

leodube-aot commented 10 months ago

OK, you got it down to a unit test error. That's progress. And if we decide to give up, we can forgo the unit tests for now (but we'd create a Tech Debt ticket to fix them later).

It also fails before any of the tests are run (see the test summary copied below, I'm running the tests only for DatePicker.spec.ts so that's why there's only one test file). I imagine it might be some issue with the test setup that's thrown from the component. One thing I'm confused by is the mounting of the component only occurs within the tests themselves, but based on the output below none of them get a chance to run.

I'll keep working on it for another hour or so, and if I don't make any more progress I'll create a Tech Debt ticket to fix it later.

Test Files  1 failed (1)
Tests  no tests
Start at  13:14:26
Duration  2.06s

leodube-aot commented 10 months ago

Created new techdebt ticket for fixing up the unit tests. I'll get one more review before merging this PR.

https://app.zenhub.com/workspaces/entities-team-space-6143567664fb320019b81f39/issues/gh/bcgov/entity/18943

ketaki-deodhar commented 10 months ago

can you try passing the mixin in the mount? Not sure if it will work or if right way to do it but something like below..... !

No luck unfortunately. I passed the mixin to the mount in the unit tests like so:

I ran it with the previous method of adding the mixin to the component, which doesn't seem to register the mixin based on the error message. In this case, both storybook and the unit tests fail.

I also tried running it with the way I extend the mixin in this PR. In this case storybook works but the unit tests fail.

Unit tests are a big pain point :(

leodube-aot commented 10 months ago

Unit tests are a big pain point :(

I agree haha I felt like I was going in circles yesterday