wegue-oss / wegue

Template and components for webmapping applications with OpenLayers and Vue.js
BSD 2-Clause "Simplified" License
97 stars 42 forks source link

Update dependencies prior to Wegue v2 #335

Closed spwoodcock closed 1 year ago

spwoodcock commented 1 year ago

Updated any remaining non-dev dependencies, on top of all previous PRs in the V2 series. Replaces #295.

I can't run the Karma tests easily on my system & don't have time for a workaround right now. It would be worth running npm run test after all the updates.

chrismayer commented 1 year ago

I just checked out the changeset of this PR and created a new Wegue app with npm run init:app. Unfortunately this layout seems to be a bit off. There seems to be an Element (white) on top of the map, which shifts down the map itself:

image

Also for the sidebar layout:

image

EDIT: Within PR #320 everything seems to be OK. The problem seems to be introduced with PR #333 and upwards.

spwoodcock commented 1 year ago

Thanks @chrismayer for the feedback - a bit silly of me - perhaps these are changes introduced by the Vuetify upgrade. I will test further next week and update 👍

spwoodcock commented 1 year ago

Found the issue in custom css for AppLogo.vue:

  .v-avatar.v-avatar--tile.wgu-app-logo {
    position: absolute;
    z-index: 2;
  }

Should be replace with:

  .v-avatar.wgu-app-logo {
    position: absolute;
    z-index: 2;
  }

And the prop rounded="0" passed to v-avatar to have a square tile.

I will update all PRs.

spwoodcock commented 1 year ago

I also updated some other minor deprecations from Vuetify 2.2.x --> 2.3.x in commit https://github.com/wegue-oss/wegue/pull/333/commits/42ead057b461b77b84e6875800ee6d1ef7fd3ea0

There were no relevant upgrades from 2.3.x --> 2.4.x and no upgrades at all from 2.4.x --> 2.6.x (LTS).

I cherry-picked the commit onto all subsequent stacked PRs.

The PR should be ready - please test again 😄

spwoodcock commented 1 year ago

It should also be noted that all edits in https://github.com/wegue-oss/wegue/commit/42ead057b461b77b84e6875800ee6d1ef7fd3ea0 were to the components directory, so no manual updates during the V1 --> V2 upgrade should be required.

There is however one minor deprecation to a file in the app-starter directory, found in commit https://github.com/wegue-oss/wegue/pull/335/commits/a12cd0d4dcf7d79c7ac8e67a3152768aa1b15519:

The update is a very simple edit to WguAppTemplate.vue.

<v-content app>
...
<v-content />

to

<v-main app>
...
<v-main />

This should probably be noted for any upgrade guide we may include for users migrating apps V1 -> V2.

chrismayer commented 1 year ago

I just pulled your updates and changed <v-content> to <v-main> in WguAppTemplate.vue. Now everything looks good to me:

image

Very nice! Thanks again.

chrismayer commented 1 year ago

@spwoodcock Would you mind resolving the conflict, so this PR gets mergable again? In my point of view the base PR #320 also has the conflict and should be an whitespace change only.

fschmenger commented 1 year ago

Hi @spwoodcock, thanks for your ongoing efforts! It looks like the merge in 6d9c8579a6b7cdee5be773e61024a2af143274b4 has gone the wrong way, as a lot of those methods in AttributeTable were removed in the master (see 3d7116395049b1f03a640824b0fb0a9511e8107e) Sorry for the brief comment, these are currently busy days for me but I hope I can test it more throughoutly in the upcoming weeks. Cheers, Felix

spwoodcock commented 1 year ago

Hey @fschmenger sorry about the notifications - I was testing to see what would fix the conflicts, as I didn't want to rebase the entire branch again.

I ended up just checking out to master for the files, as they are only whitespace related.

Please check everything is all good 👍

spwoodcock commented 1 year ago

Cherry-picked 23c9451d425646de04879c00e66850f91869d13e to make this mergable.

spwoodcock commented 1 year ago

Merged master to only show specific commits - ready to test and merge. Please add the Wegue V2 label. I'm happy to rebase to make the history clearer - let me know.

Note: as this PR is a combination of #333 and #334 (+2b5c0638365b6f6095f7cb7bc15501548bacfb30) and has been tested / approved, it's probably easiest to merge this & close the other two 👍

chrismayer commented 1 year ago

Thanks for merging in the current master @spwoodcock.

Unfortunately the unit tests in the CI fail. Not sure if this was the case before the merge or if they were OK before. They also fail for #333 and #334 and it is locally reproducible when you execute npm run test with the code of this PR.

I did a fresh checkout of the code of this PR and created the Wegue example apps. There are some linting problems in the AttributeTableWin component, which can easily be fixed with this commit.

After that everything worked as expected at least as far as I could see in some visual tests by clicking through the modules in the example apps.

spwoodcock commented 1 year ago

Thanks @chrismayer!

Apologies for not being very thorough and checking the tests after merge - I'm also pretty pushed for time currently.

We planned to use Wegue as our geospatial module in EnviDat/WSL in CH, but I am leaving and priorities have shifted for now.

I'll make contributions to fix these PRs and add cloud native format support (COG, flatgeobuf, geoparquet, COPC) in the near future.

chrismayer commented 1 year ago

@spwoodcock I had a look at the test suite and why it is failing. This commit should fix it. Are you willing to pull it into this PR or should I open a new PR based on this PR with the additional fix?

spwoodcock commented 1 year ago

Much appreciated @chrismayer! I added the commit =)

chrismayer commented 1 year ago

So I did some more tests by upgrading a current application to this branch and it works very well. No problems so far, so I finally will merge this :tada:

If any problems will occur afterwards we will handle them in follow-up PRs as always.

I also will come up with an update of the migration guide and I am going to update the v2 roadmap (#327).

Thanks again to all, who were involved. This is another great step forward :+1: