2 add harpgl support - Githubissues

GeospatialResearch / gri-earthquake

Other

1 stars 0 forks source link

2 add harpgl support #7

Closed LukeParky closed 3 years ago

LukeParky commented 3 years ago

Adds the Harp.GL mapping library to the project. By following the README contributors should be able to have a map shown on the application.

rosepearson commented 3 years ago

Good clear readme instructions and clear overall directory and file structure. 👍

rosepearson commented 3 years ago

The code changes look good to me. I've going to have a look through some of the associated files - i.e. from previous commits prior to the PR, before finishing up.

In https://github.com/LukeParky/gri-earthquake/blob/2-add-harpgl-support/client/src/pages/AboutPage.vue worth updating to GitHub link.
Add doc strings to server/app.py

I've looked from the vue and js files and generally they seem well structured and clear from clutter. I did wonder if any comments could be added to them? I also wondered it it makes sense to add a mention in the ReadMe to the client/server structure and the fact you're using docker.

LukeParky commented 3 years ago

Still to do for this PR:

~~Make nested README structure or additional .md files for instructions~~
~~Increase in-code documentation~~
~~Add screenshots to the README~~
~~Update AboutPage to github~~
~~Hide decoder config files away from root directory~~
~~Add work in progress notes on README~~
~~Resolve comments~~

LukeParky commented 3 years ago

Hi @LukeParky, your changes look good to me. I like the comments and child readme's. It's made everything a lot clearer to me. Nice work.

The only think I notice is a few warnings about files ending without a new line i.e. client/decoders/harp-gl-decoder.config.js - but i'll leave that to your discretion if you address this now or not.

Thanks Rose, I'll make those changes and merge