Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dependencies prior to Wegue v2 #335

Merged
merged 14 commits into from
Jul 24, 2023

Conversation

spwoodcock
Copy link
Contributor

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
Copy link
Collaborator

chrismayer commented May 8, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

spwoodcock commented May 9, 2023

I also updated some other minor deprecations from Vuetify 2.2.x --> 2.3.x in commit 42ead05

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
Copy link
Contributor Author

spwoodcock commented May 9, 2023

It should also be noted that all edits in 42ead05 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 a12cd0d:

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
Copy link
Collaborator

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
Copy link
Collaborator

@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.

@envidat-git envidat-git force-pushed the build/update-dependencies-v2 branch from 4309331 to 6d9c857 Compare May 11, 2023 08:16
@fschmenger
Copy link
Collaborator

Hi @spwoodcock, thanks for your ongoing efforts!
It looks like the merge in 6d9c857 has gone the wrong way, as a lot of those methods in AttributeTable were removed in the master (see 3d71163)
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

@envidat-git envidat-git force-pushed the build/update-dependencies-v2 branch from 6d9c857 to 1cb95cc Compare May 11, 2023 08:38
@spwoodcock
Copy link
Contributor Author

spwoodcock commented May 11, 2023

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
Copy link
Contributor Author

Cherry-picked 23c9451 to make this mergable.

@spwoodcock
Copy link
Contributor Author

spwoodcock commented Jul 5, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

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 added a commit to chrismayer/wegue that referenced this pull request Jul 18, 2023
This fixes the test suite due to the library / dependency updates in PR wegue-oss#335.
@chrismayer
Copy link
Collaborator

@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?

This fixes the test suite due to the library / dependency updates in PR wegue-oss#335.
@spwoodcock
Copy link
Contributor Author

Much appreciated @chrismayer! I added the commit =)

@chrismayer
Copy link
Collaborator

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 🎉

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 👍

@chrismayer chrismayer merged commit 1cf2cfa into wegue-oss:master Jul 24, 2023
chrismayer added a commit to chrismayer/wegue that referenced this pull request Jul 24, 2023
This adds some essentials, which have to be done when upgrading a Wegue
application to the lastest master branch, which will become the new v2
soon.
chrismayer added a commit that referenced this pull request Jul 24, 2023
@spwoodcock spwoodcock deleted the build/update-dependencies-v2 branch July 24, 2023 20:48
chrismayer added a commit to chrismayer/wegue that referenced this pull request Aug 14, 2023
This fixes the package-lock.json, which was messed up since merging the
PR wegue-oss#335. The file is re-created with Node.js v16 and npm in v8.
@chrismayer chrismayer mentioned this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants