boney-bun commented 5 years ago

fix #kartoza/geosafe#531

NyakudyaA commented 5 years ago

@boney-bun does this remove the base maps even at Layer detail page, Because this also affects the thumbnail generation where the basemap is empty

codecov[bot] commented 5 years ago

Codecov Report

Merging #537 into 2.8.x will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##            2.8.x     #537   +/-   ##
=======================================
  Coverage   57.75%   57.75%           
=======================================
  Files          49       49           
  Lines        2720     2720           
  Branches      301      301           
=======================================
  Hits         1571     1571           
  Misses       1068     1068           
  Partials       81       81

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 08db72a...0a89ffe. Read the comment docs.

NyakudyaA commented 5 years ago

Why we don't use OSM as default basemap? I'm not sure why you add Terrain in this page? Could you use the same logic from Layer details to include set of basemaps taken from the settings file?

Yes @lucernae we just need to have a basemap that we know always works. OSM basemap would be nice to have it as the default and the terrain can still be added as a second option for people who do want to change

lucernae commented 5 years ago

Yes @lucernae we just need to have a basemap that we know always works. OSM basemap would be nice to have it as the default and the terrain can still be added as a second option for people who do want to change

Yeah, argh sorry for not being clear :D. If I read my comment above again, it seems I'm complaining about the Terrain map. What I mean is, we should add the Terrain in the settings, not in the page. Then take the default basemap from whatever the first basemap in the list from the settings.

In last year's sprint, we already fixed this problem by offloading the basemap to settings, so we made this:

https://github.com/kartoza/geonode/blob/2.8.x-qgis_server/geonode/client/templates/leaflet/layers/layer_leaflet_map.html#L142 (in kartoza/geonode repo)

The layer detail page takes extra basemaps from the settings module.

I suggest that:

We take the same approach for analysis related page (because it isn't handled by geonode) in this geosafe repo.
Customize the basemaps to everywhere from settings module.
Add new settings field to choose default basemaps from a given basemaps defined in settings
Refactor layer and map page of GeoNode to choose default basemaps from settings.

The settings that I was talking about:

https://github.com/kartoza/geonode/blob/2.8.x-qgis_server/geonode/settings.py#L1015

It would be nice if we can handle all these

boney-bun commented 5 years ago

yes, the default is osm. terrain is just an additional basemap to replace the removed one. i have another PR in custom repo. i thought we will just use the custom one. i guess i will close the PR in the custom repo and move it to geonode repo. i also will revise this PR.

boney-bun commented 5 years ago

the PR in action: geosafe#531-geosafe

the basemaps are taken from settings. the default basemap is always osm.

@lucernae @NyakudyaA could you review again?

boney-bun commented 5 years ago

thanks for the feedback @lucernae @NyakudyaA i've revised and tested it locally. i'm going to merge this PR.

kartoza / geosafe

change OpenMapSurfer_Roads basemap to terrain #537

Codecov Report