-
Notifications
You must be signed in to change notification settings - Fork 41
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
Arcgis rest layers support #371
Conversation
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.
Thanks for this nice feature @enricofer! I like. I am not really into deep into the ArcGIS stuff but it looks good to me. I second this 👍
The PR has a conflict, which needs to be resolved and I had some smaller remarks mainly on textual level (API Docs et.al.). Before merging it would also be cool, if you could provide some documentation for this new layer type at https://github.com/wegue-oss/wegue/blob/master/docs/map-layer-configuration.md. This will then automatically show at https://wegue-oss.github.io/wegue/#/map-layer-configuration
app-starter/locales/en.json
Outdated
@@ -57,6 +57,10 @@ | |||
"terrestris-osm-wms" : { | |||
"name": "OSM WMS", | |||
"attributions": "<a href='https://www.openstreetmap.org/copyright' target='_blank'>© OpenStreetMap-contributors</a>" | |||
}, | |||
"test_arcgisrest" : { | |||
"name": "unità urbane", |
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.
Could you please provide an English text / layer name here?
src/factory/Layer.js
Outdated
@@ -165,6 +168,31 @@ export const LayerFactory = { | |||
return layer; | |||
}, | |||
|
|||
/** | |||
* Returns an OpenLayers Tiled WMS layer instance due to given config. |
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.
Wrong API docs, maybe copy/paste?
src/factory/Layer.js
Outdated
* Returns an OpenLayers Tiled WMS layer instance due to given config. | ||
* | ||
* @param {Object} lConf Layer config object | ||
* @return {ol.layer.Tile} OL Tiled WMS layer instance |
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.
Wrong API docs, maybe copy/paste?
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.
Thanks for your ongoing effort @enricofer ! I found 2 minor things which should be addressed before we can merge this.
I just finalized this by committing the 2 minor issues of my last review. This is good to go now. Thanks again @enricofer for providing this new layer type. |
This PR is about the support for Arcgis REST layers https://openlayers.org/en/v8.1.0/apidoc/module-ol_source_TileArcGISRest-TileArcGISRest.html