Introduces WfsSearch #563

KaiVolland · 2018-04-09T07:13:48Z

FEATURE

Description:

This introduces the WfsSearch.

annarieger

LGTM, I've added some mostly minor notes regarding to docs.

Please merge at will and thanks 🍍

annarieger · 2018-04-10T09:32:01Z

src/Field/WfsSearch/WfsSearch.jsx

+     */
+    srsName: PropTypes.string,
+    /**
+     * Ther output format of the response.


Typo Ther -> The

annarieger · 2018-04-10T09:42:37Z

src/Field/WfsSearch/WfsSearch.jsx

+    baseUrl: PropTypes.string.isRequired,
+    /**
+     * The Property which should be shown in the inputfield when a selection was
+     * made.


very minor: here and by some further props below @type annotation is missing

annarieger · 2018-04-10T09:44:43Z

src/Field/WfsSearch/WfsSearch.jsx

+     * @return {AutoComplete.Option} The AutoComplete.Option that will be
+     * rendered for each feature.
+     */
+    renderOption: (feature) => {


very minor: () around the argument can be ommited

annarieger · 2018-04-10T09:45:14Z

src/Field/WfsSearch/WfsSearch.jsx

+     * The default onSelect method if no onSelect prop is given. It zooms to the
+     * selected item.
+     *
+     * @param {object} feature The selected feature as returned by the server.


olMap param missing

annarieger · 2018-04-10T09:49:28Z

src/Field/WfsSearch/WfsSearch.jsx

+
+  /**
+   * This function gets called when the nomintim fetch returns an error.
+   * It logs the error to the console.


Please adapt method description

annarieger · 2018-04-10T09:51:17Z

src/Field/WfsSearch/WfsSearch.spec.jsx

+      setTimeout(() => {
+        expect(map.getView().getCenter()).toEqual([25, 25]);
+        expect(map.getView().getZoom()).toEqual(2);
+      }, 501);


ahennr

👏 nice work @KaiVolland

Added some remarks you might want to tackle.

ahennr · 2018-04-10T12:39:07Z

src/Field/WfsSearch/WfsSearch.jsx

+    } = this.props;
+
+    const propertyFilters = searchAttributes.map(attribute =>
+      OlFormatFilter.like(attribute, `*${searchTerm}*`, '*', '.', '!', false));


What happens here if numerical / boolean attributes are filtered / queried here?

Uff i don't know. I think this depends on the WFS-Server. Either it throws an error or it just won't match... Anything we can do about it?

for geoserver we can cast e.g. by using a string function like concatenate with an empty string, which will accept any input (according to the docs). this should be configurable defaulting to false probably.

ahennr · 2018-04-10T12:43:34Z

src/Field/WfsSearch/WfsSearch.jsx

+      filter: this.createFilter()
+    };
+
+    const wfsFormat = new OlFormatWFS();


By default OlFormatWFS supports WFS version 1.1. Please make the used WFS version configurable (see e.g. here).

WFS 2.0.x is not supported, or am I wrong?

ol.format.WFS does not support WFS 2.0.0, is it really necessary to support WFS 1.0.0?

no need for 1.0.0 IMO. 2.0.0 would be nice, but is not a must have.

Since WFS 1.0.0 will be generated simply by using another GML format in OlFormatWFS I would make this configurable

I 👒 this done in 638c12d

ahennr · 2018-04-10T12:45:54Z

src/Field/WfsSearch/WfsSearch.jsx

+      credentials: additionalFetchOptions.credentials
+        ? additionalFetchOptions.credentials
+        : 'same-origin',
+      body: new XMLSerializer().serializeToString(featureRequest),


I've in mind that there existed some problems using XMLSerializer in Firefox.

Did you check this also other browsers like FF, Edge, ..?

No i didn't nice hint. I'll check this.

After a quick check, it works as aspected in FireFox and Internet-Explorer.

ahennr · 2018-04-10T12:49:20Z

src/Field/WfsSearch/WfsSearch.jsx

+     * The base URL.
+     * @type {String}
+     */
+    baseUrl: PropTypes.string.isRequired,


Must CORS headers be set by the WFS server?

Yes, i think so. I'll add it to the documentation.

marcjansen

Nice work. please take from my comments what you think is appropriate.

marcjansen · 2018-04-11T06:32:10Z

src/Field/WfsSearch/WfsSearch.jsx

+     * The list of attributes that should be searched through.
+     * @type {String[]}
+     */
+    searchAttributes: PropTypes.arrayOf(PropTypes.string).isRequired,


Can we get a utility to get these from a DescribeFeatureType? Posibbly first fetching these and filtering to those usable?

Sounds very useful. Can you open an issue for it?

Here you are: #567

marcjansen · 2018-04-11T06:34:11Z

src/Field/WfsSearch/WfsSearch.jsx

+     * made.
+     * @type {String}
+     */
+    displayField: PropTypes.string,


Can we change this to be a template or a function, so I can show multiple fields and arbitrary text? Also it would be super nice if we could somehow mark the matched textpart in the property.

After having a look at the code i found out, that this prop is never used. I'll remove it. All what you expect should be realized in the renderOption prop.

marcjansen · 2018-04-11T06:35:17Z

src/Field/WfsSearch/WfsSearch.jsx

+     * Geometry name to use in a BBOX filter.
+     * @type {string}
+     */
+    geometryName: PropTypes.string,


A utility to get this from a DescribeFeatureType would be nice

Sounds very useful. Can you open an issue for it?

Here you are: #567

marcjansen · 2018-04-11T06:36:02Z

src/Field/WfsSearch/WfsSearch.jsx

+     *
+     * @type {Object}
+     */
+    map: PropTypes.instanceOf(OlMap).isRequired,


Why is this required? we can search without a map I guess?

marcjansen · 2018-04-11T06:37:33Z

src/Field/WfsSearch/WfsSearch.jsx

+     * @return {AutoComplete.Option} The AutoComplete.Option that will be
+     * rendered for each feature.
+     */
+    renderOption: feature => {


If a user can provide this, we should list actual requirements, if any. I am thinking of the id property, e.g.

I'll enhance the docs at the property.

marcjansen · 2018-04-11T06:38:33Z

src/Field/WfsSearch/WfsSearch.jsx

+        if (geometry) {
+          olView.fit(geometry, {
+            duration: 500
+          });


These options might be a good candidate for configuration, right?

As it's just in the default of the onSelect property this doesn't need to be configurable. You can just provide a complete new function if you want a different duration.

marcjansen · 2018-04-11T06:39:19Z

src/Field/WfsSearch/WfsSearch.jsx

+
+    if (value) {
+      this.setState({
+        searchTerm: value || ''


|| '' seems unneeded

marcjansen · 2018-04-11T06:40:09Z

src/Field/WfsSearch/WfsSearch.jsx

+      filter: this.createFilter()
+    };
+
+    const wfsFormat = new OlFormatWFS();


no need for 1.0.0 IMO. 2.0.0 would be nice, but is not a must have.

marcjansen · 2018-04-11T06:41:40Z

src/Field/WfsSearch/WfsSearch.jsx

+    } = this.props;
+
+    const propertyFilters = searchAttributes.map(attribute =>
+      OlFormatFilter.like(attribute, `*${searchTerm}*`, '*', '.', '!', false));


for geoserver we can cast e.g. by using a string function like concatenate with an empty string, which will accept any input (according to the docs). this should be configurable defaulting to false probably.

marcjansen · 2018-04-11T06:42:59Z

src/Field/WfsSearch/WfsSearch.spec.jsx

+      expect(fitSpy).toHaveBeenCalled();
+      setTimeout(() => {
+        expect(map.getView().getCenter()).toEqual([25, 25]);
+        expect(map.getView().getZoom()).toEqual(2);


is this actually tested?

expect.assertions is missing. I'll add it.

…-jest-27.4.5 Bump babel-jest from 27.4.4 to 27.4.5

KaiVolland force-pushed the wfs-search branch from 0fa5b04 to 6c0b278 Compare April 9, 2018 12:54

Introduces WfsSearch

8e12eb4

KaiVolland force-pushed the wfs-search branch from 6c0b278 to 8e12eb4 Compare April 9, 2018 14:05

KaiVolland changed the title ~~[WIP] Introduces WfsSearch~~ Introduces WfsSearch Apr 9, 2018

KaiVolland changed the title ~~Introduces WfsSearch~~ Introduces WfsSearch Apr 9, 2018

KaiVolland mentioned this pull request Apr 9, 2018

WFS Search #555

Closed

annarieger approved these changes Apr 10, 2018

View reviewed changes

ahennr requested changes Apr 10, 2018

View reviewed changes

marcjansen approved these changes Apr 11, 2018

View reviewed changes

Enhance documentation for WfsSearch

b1e6199

KaiVolland force-pushed the wfs-search branch from fd4da69 to b1e6199 Compare April 11, 2018 08:06

Adds wfsFormatOptions prop to WfsSearch

f9830be

KaiVolland force-pushed the wfs-search branch from 638c12d to f9830be Compare April 11, 2018 09:23

ahennr approved these changes Apr 11, 2018

View reviewed changes

KaiVolland merged commit e7907ca into terrestris:master Apr 11, 2018

KaiVolland deleted the wfs-search branch April 11, 2018 11:14

marcjansen mentioned this pull request Apr 12, 2018

WFS DescribeFeature utility methods #567

Open

KaiVolland restored the wfs-search branch November 22, 2019 09:00

hblitza pushed a commit that referenced this pull request Oct 19, 2022

Merge pull request #563 from terrestris/dependabot/npm_and_yarn/babel…

010af75

…-jest-27.4.5 Bump babel-jest from 27.4.4 to 27.4.5

Introduces WfsSearch #563

Introduces WfsSearch #563

Conversation

KaiVolland commented Apr 9, 2018

FEATURE

Description:

annarieger 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

ahennr 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

KaiVolland Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaiVolland Apr 10, 2018 • edited Loading

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

KaiVolland Apr 11, 2018 • edited Loading

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

KaiVolland Apr 11, 2018 • edited Loading

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

KaiVolland Apr 11, 2018 •

edited

Loading

KaiVolland Apr 10, 2018 •

edited

Loading

KaiVolland Apr 11, 2018 •

edited

Loading

KaiVolland Apr 11, 2018 •

edited

Loading