-
-
Notifications
You must be signed in to change notification settings - Fork 489
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
Map viewer / Remove Stamen background layers - no longer available #7715
Conversation
How urgent is this? Are maps broken presently? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull-request is successful in its stated aim of removing stamen. I understand that stadiamaps is not yet available to the project.
If I am reading the code correctly configurations previously created will now display an empty background?
Suggestion: Ideally we could offer a placeholder for stamen until we were in a position to use strradiamaps. This way existing configurations would continue to do something until we upgrade. As an example we could display OSM in grayscale to provide something for context.
@@ -177,7 +177,6 @@ This section is for configuring the map shown when viewing a record. | |||
- **wmts**: generic WMTS layer, required properties: `name, url`. | |||
- **tms**: generic TMS layer, required property: `url`. | |||
- **osm**: OpenStreetMap default layer, no other property required. | |||
- **stamen**: Stamen layers, required property: `name`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be adding stadiamaps here at the same time we remove stamen ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of OpenLayers used in GeoNetwork doesn't support StadiaMaps. We can upgrade OpenLayers, there is a pull request on-going #7421, but not sure about the status and if it's even possible with the GeoNetwork code to do such upgrade.
return new ol.layer.Tile({ | ||
source: source, | ||
title: title || "Stamen" | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This switch/case does not appear to have a default...
If we are removing, rather than offering stadiamaps alternative we should default to osm or something that will display rather than being empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSM is the default AFAIK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
The backport to
stderr
stdout
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-3.12.x 3.12.x
# Navigate to the new working tree
cd .worktrees/backport-3.12.x
# Create a new branch
git switch --create backport-7715-to-3.12.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 4343e99e59f4ca7282cdb6b0963e3243162d435f
# Push it to GitHub
git push --set-upstream origin backport-7715-to-3.12.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-3.12.x Then, create a pull request where the |
It may have been wise to at least show something (e.g. a StadiaMaps layer) instead of leaving the map empty. And StadiaMaps are simple XYZ layers so a new version of OL is not really needed to show them; OL 8 only gives a convenience class to use it. |
@jahow for now only OSM is available with this change. It can be done another PR to add the StadiaMaps using XYZ layers. |
I mean if someone loads a map context with a stamen layer in it, it won't show up right? |
I haven't try that, but I don't think it will be processed the stamen layer. |
That is what I was trying to ask in my feedback. |
@jodygarnett not sure if I understand your last comment. The default map context file in GeoNetwork has been updated to offer only OSM, to avoid listing also the Stamen layers that doesn't work. If a user loads a map context file with Stamen layer, or other type of layer not supported in GeoNetwork, we don't offer any fallback, the code AFAIK just ignores that type of layer. If we want to offer this kind of fallback, we need to modify the code that loads the map context files in GeoNetwork, but I'm afraid that is out of the scope of this change request. |
I understand it is out of scope; we will offer update instructions. Or is fixing this part of a migrating process? Because users make and save these context files in the catalogue… |
Stamen layers are no longer available, have been replaced by StadiaMaps.
New versions of OpenLayers support StadiaMaps: https://openlayers.org/en/latest/examples/stamen.html, but the version used in GeoNetwork doesn't have support for it.
For the time being, this change request removes Stamen layers.
Checklist
main
branch, backports managed with labelREADME.md
filespom.xml
dependency management. Update build documentation with intended library use and library tutorials or documentation