powa-team / powa-web

PoWA user interface
http://powa.readthedocs.io/
74 stars 31 forks source link

Upgrade to Vue3 #190

Closed pgiraud closed 11 months ago

pgiraud commented 11 months ago

The goal of this PR is to use Vue version 3.x instead of 2.7.

It was not possible before because we had to wait for Vuetify3 to be mature enough, which happened with Vuetify3.4 with the addition of VDataTable and VDatePicker.

A important improvement has been done on how global data is managed.

rjuju commented 11 months ago

Thanks!

What do you mean by global data management?

I will need a bit of time to review that PR. But after very minimal testing I see that the issue with the browser navigation button (go back and go forward) seems to be back.

pgiraud commented 11 months ago

Thanks!

What do you mean by global data management?

The way the data is managed in the JS is simplified. The root App component is responsible for loading the data and provides this data to its child components. This is much simpler than trying to use a composable (VueJS concept) like we did before.

I will need a bit of time to review that PR. But after very minimal testing I see that the issue with the browser navigation button (go back and go forward) seems to be back.

Fixed.

rjuju commented 11 months ago

I did a first general round of review. First of all, as far as I can see all features are working as expected, that's nice :) The rest is just glitches or other minor issues, in no specific order:

Other than that, do we really need 22 commits for this change or are you planning to do some fixup before merging?

pgiraud commented 11 months ago

Thanks for this detailed list of problems.

All have now been taken into account. With additional layout improvements along the way. For example, I made the UI more compact but reducing the extra spaces as much as possible but still trying to make it readable.

Other than that, do we really need 22 commits for this change or are you planning to do some fixup before merging?

I'm actually in favor of squashing commits as much as possible. I'll do it once this is validated.

rjuju commented 11 months ago

All have now been taken into account. With additional layout improvements along the way. For example, I made the UI more compact but reducing the extra spaces as much as possible but still trying to make it readable.

Thanks, I will have another round of review shortly! I saw in the new commits that you had some improvements on making things more compact, that's quite appreciated!

I'm actually in favor of squashing commits as much as possible. I'll do it once this is validated.

Ok! Note that I'm still in favor of keeping extra commits when such a separation helps with the clarity of the git history. In any case agreed that's clearly not a priority for now.

rjuju commented 11 months ago

After another round of comparison testing multiple scenario it all looks good to me, thanks a lot! I quite appreciate that we can see much more information now. Maybe some people will find it too small or something, but in any case that would be out of the scope for this PR.

I will let you rebase the branch to cleanup the history, after that feel free to merge or just ask me for another review if you prefer me to take care of it!