Introduce AddWmsPanel #267

ahennr · 2017-12-05T13:40:34Z

Container
AddWmsPanel -- Panel containing a (checkable) list of AddWmsLayerEntry instances
AddWmsLayerEntry -- Visual representation of layer entry parsed from capabilities document.
adds an example which parses capabilities document of OWS terrestris

Plz review.

KaiVolland

Nice work.

I added some notes regarding a CapabilitiesUtil. Please have a look at it.

The other notes are noneblocking.

KaiVolland · 2017-12-05T13:47:05Z

src/Container/AddWmsPanel/AddWmsLayerEntry/AddWmsLayerEntry.jsx

+      wmsAttribution: PropTypes.string,
+      queryable: PropTypes.boolean,
+      Abstract: PropTypes.string,
+      Title: PropTypes.string


Is uppercase intended?

Yes, this matches to notation in the object parsed by ol WMS Capabilities parser

KaiVolland · 2017-12-05T13:48:29Z

src/Container/AddWmsPanel/AddWmsLayerEntry/AddWmsLayerEntry.jsx

+      copyright: props.wmsLayer.wmsAttribution,
+      queryable: props.wmsLayer.queryable
+    };
+  }


Do we need a componentWillReceiveProps or is it unlikely that the AddWmsLayerEntry will receive props after creation?

IMHO it's very unlikely that the AddWmsLayerEntry will receive updated props after creation.

KaiVolland · 2017-12-05T13:58:21Z

src/Container/AddWmsPanel/AddWmsPanel.example.jsx

+      const wmsGetFeatureInfoConfig = get(caps, 'Capability.Request.GetFeatureInfo');
+
+      return get(caps,'Capability.Layer.Layer').map(layer => Object.assign(layer,  {wmsVersion, wmsAttribution, wmsGetMapConfig, wmsGetFeatureInfoConfig}) );
+    }).then((wmsLayers) => {


Would be very nice if you could extract this method into small functions in something like a CapabilitiesUtil. We discussed this some days ago.
I think it will be very usefull in many applications.

Added in 823294f

KaiVolland · 2017-12-05T14:09:07Z

src/Container/AddWmsPanel/AddWmsPanel.jsx

+     * parser)
+     * @type {Array} -- required
+     */
+    wmsLayers: PropTypes.arrayOf(PropTypes.object).isRequired,


I think it would be better to work with instances of OlLayer here. The parser could be in the CapabilitiesUtil described above. I don't like the idea of working with diffrent "layerlike" objects across the framework.
You could then implement an "real" addToMap method wich would call the property method onLayerAddToMap after add. Ofcourse the map should be a required prop.

I replaced all "custom" objects representing a layer by an instance of OlLayerTile.

See c2889cb

marcjansen

Nice work, @ahenn. I have added a couple of remarks, you decide what to do with them. Support for 1.3.0 is essential, IMO.

marcjansen · 2017-12-05T20:20:22Z

src/Container/AddWmsPanel/AddWmsLayerEntry/AddWmsLayerEntry.jsx

+    const abstract = wmsLayer.get('abstract');
+    const abstractTextSpan = abstract ?
+      <span>{`${title} - ${abstract}:`}</span> :
+      <span>{`${title}`}</span>;


Can we have this configurable? with a template or a metjod or ...? What do you think?

marcjansen · 2017-12-05T20:23:15Z

src/Container/AddWmsPanel/AddWmsLayerEntry/AddWmsLayerEntry.spec.jsx

+    testLayer.set('queryable', false);
+  });
+
+  it('doesnt add queryable icon if prop wmsLayer has queryable set to false', () => {


missing apostrophe in doesnt: doesn't

marcjansen · 2017-12-05T20:23:44Z

src/Container/AddWmsPanel/AddWmsLayerEntry/AddWmsLayerEntry.spec.jsx

+    expect(icons.getElement().props.className).toContain('fa-copyright');
+  });
+
+  it('doesnt add copyright icon if prop wmsLayer no has filled attribution', () => {


marcjansen · 2017-12-05T20:24:22Z

src/Container/AddWmsPanel/AddWmsPanel.example.jsx

+//
+
+// Please note: CORS headers must be set on server providing capabilities document. Otherwise proxy needed.
+const WMS_CAPABILITIES_URL = 'http://ows.terrestris.de/osm/service?SERVICE=WMS&VERSION=1.1.1&REQUEST=GetCapabilities';


marcjansen · 2017-12-05T20:25:58Z

src/Container/AddWmsPanel/AddWmsPanel.example.md

+---
+layout: basic.html
+title: AddWmsPanel example
+description: This is a example using a AddWmsPanel.


an example ... using the AddWmsPanel

marcjansen · 2017-12-05T20:26:15Z

src/Container/AddWmsPanel/AddWmsPanel.example.md

+collection: Examples
+---
+
+This is a example containing a AddWmsPanel


marcjansen · 2017-12-05T20:30:50Z

src/Util/CapabilitiesUtil/CapabilitiesUtil.js

+      .then((data) => {
+        const wmsCapabilitiesParser = new OlWMSCapabilities();
+        return wmsCapabilitiesParser.read(data);
+      });


How are errors handled?

These are handled by the method / statement handling Promise returned by parseWmsCapabilities. I've added an example in AddWmsLayerEntry.example.jsx

I think adding a seperate error handler to every promise we use would bei even saver, but we don't do it anywhere. Lets open an issue therefore.

marcjansen · 2017-12-05T20:31:50Z

src/Util/CapabilitiesUtil/CapabilitiesUtil.js

+    return layersInCapabilities.map((layerObj) => new OlLayerTile({
+      opacity: 1,
+      title: get(layerObj, 'Title'),
+      abstract: get(layerObj, 'Attribution'),


is abstract == attribution?

No. Replaced by Abstract. For each layer, the abstract property in service/layer metadata must be provided properly in order to be shown here.

marcjansen · 2017-12-05T20:32:29Z

src/Util/CapabilitiesUtil/CapabilitiesUtil.js

+      abstract: get(layerObj, 'Attribution'),
+      getFeatureInfoUrl: getFeatureInfoUrl,
+      queryable: !!getFeatureInfoUrl,
+      source: new OlSourceTileWms({


Always tiled?

I've changed this to OlSourceImageWMS. IMHO we should return tiled layers in a further method which parses capabilities of WMTS services.

marcjansen · 2017-12-05T20:33:11Z

src/Util/CapabilitiesUtil/CapabilitiesUtil.spec.js

+
+const layerTitle =  'OpenStreetMap WMS - by terrestris';
+const capabilitiesObj = {
+  version: '1.1.0',


Do we also support 1.3.0?

Yes we do since the OL capabilities parser does. I changed version this in this object used for testing getLayersFromWmsCapabilties.

marcjansen

One other thing: adding the dist folder makes reading the diff hard. Can we agree not to do that?

ahennr · 2017-12-06T13:01:57Z

@KaiVolland: Do you think this is ok now?

* Container * AddWmsPanel -- Panel containing a (checkable) list of AddWmsLayerEntry instances * AddWmsLayerEntry -- Visual representation of layer entry parsed from capabilities document. * adds an example which parses capabilities document of OWS terrestris

* adds template function for textual representation of layer * use https and WMS v1.3.0 * enhance description of examples

ahennr · 2017-12-07T10:18:03Z

Thx for your reviews. I'll merge as soon as all checks are green.

KaiVolland requested changes Dec 5, 2017

View reviewed changes

ahennr force-pushed the add-wms-grid branch from fd9e80c to e3cf15c Compare December 5, 2017 14:21

marcjansen approved these changes Dec 5, 2017

View reviewed changes

marcjansen reviewed Dec 5, 2017

View reviewed changes

ahennr force-pushed the add-wms-grid branch 2 times, most recently from 7ddda0d to c1304c3 Compare December 6, 2017 12:47

KaiVolland approved these changes Dec 7, 2017

View reviewed changes

ahennr added 8 commits December 7, 2017 11:17

Introduce AddWmsPanel

0822dc3

* Container * AddWmsPanel -- Panel containing a (checkable) list of AddWmsLayerEntry instances * AddWmsLayerEntry -- Visual representation of layer entry parsed from capabilities document. * adds an example which parses capabilities document of OWS terrestris

make checkbox group scrollable

ff37d9a

adds CapabilitiesUtil

5430aaf

use OLTileWms layers instead of custom objects

d63a9f8

various enhancements of AddWmsPanel and AddWmsLayerEntry

7a14a8b

* adds template function for textual representation of layer * use https and WMS v1.3.0 * enhance description of examples

parse image WMS instead of tile WMS

a93bff0

enhance proptype validation of wmsLayers

be15c87

add GFI formats and queryable properties to layer

1e8db8a

ahennr force-pushed the add-wms-grid branch from b8200f6 to 1e8db8a Compare December 7, 2017 10:17

ahennr merged commit 7f4ec0a into terrestris:master Dec 7, 2017

ahennr deleted the add-wms-grid branch December 7, 2017 11:07

KaiVolland mentioned this pull request Jan 25, 2018

Features - RoadMap #240

Closed

56 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce AddWmsPanel #267

Introduce AddWmsPanel #267

ahennr commented Dec 5, 2017

KaiVolland left a comment

KaiVolland Dec 5, 2017

ahennr Dec 5, 2017

KaiVolland Dec 5, 2017

ahennr Dec 5, 2017

KaiVolland Dec 5, 2017

ahennr Dec 5, 2017

KaiVolland Dec 5, 2017

ahennr Dec 5, 2017

ahennr Dec 5, 2017

marcjansen left a comment

marcjansen Dec 5, 2017

marcjansen Dec 5, 2017

marcjansen Dec 5, 2017

marcjansen Dec 5, 2017

ahennr Dec 6, 2017

marcjansen Dec 5, 2017

marcjansen Dec 5, 2017

marcjansen Dec 5, 2017

ahennr Dec 6, 2017

KaiVolland Dec 6, 2017

marcjansen Dec 5, 2017

ahennr Dec 6, 2017

marcjansen Dec 5, 2017

ahennr Dec 6, 2017

marcjansen Dec 5, 2017

ahennr Dec 6, 2017

marcjansen left a comment

ahennr commented Dec 6, 2017

ahennr commented Dec 7, 2017

Introduce AddWmsPanel #267

Introduce AddWmsPanel #267

Conversation

ahennr commented Dec 5, 2017

KaiVolland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcjansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcjansen left a comment

Choose a reason for hiding this comment

ahennr commented Dec 6, 2017

ahennr commented Dec 7, 2017