Coordinate reference system combo by ahennr · Pull Request #244 · terrestris/react-geo

ahennr · 2017-11-28T16:32:32Z

This PR introduces a Combobox where uses can choose a coordinate reference system. If no object with predefined CRS definitions (predefinedCrsDefinitions) is given in props, the CRS combo can be uses as an AutoComplete field querying epsg.io - API for CRS definitions.

An example shows the usage both, using an array containing predefined values as well as fetching CRS defintions.

In order to handle projections / transformations, proj4 has been added as dependency. In addition, aProjectionUtil has been added as well.

Plz review.

marcjansen

Thanks, Rö.

Only minor comments, please merge once adressed

marcjansen · 2017-11-28T17:43:21Z

src/Field/CoordinateReferenceSystemCombo/CoordinateReferenceSystemCombo.jsx

@@ -0,0 +1,228 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import {AutoComplete } from 'antd';


Missing a space

fixed in 77710ba

marcjansen · 2017-11-28T17:43:36Z

src/Field/CoordinateReferenceSystemCombo/CoordinateReferenceSystemCombo.jsx

+import { Logger, UrlUtil } from '../../index.js';
+
+/**
+ * Class representating a combo to choose coordinate projection system via a


representing

fixed in 77710ba

marcjansen · 2017-11-28T22:09:41Z

src/Field/CoordinateReferenceSystemCombo/CoordinateReferenceSystemCombo.less

@@ -0,0 +1,3 @@
+.react-geo-crs-combo {
+  width: 200px;


why do we have a fixed width in the library?

removed less file in 77710ba. Style can be passed as prop now.

marcjansen · 2017-11-28T22:10:18Z

src/Field/NominatimSearch/NominatimSearch.jsx

-      limit,
-      countrycodes,
-      map,
-      onSelect,


huh? is this ok here?

I think this is mistake… plz revert.

I don't see where these variables are used in render function?!

They are not used but you have to extract them from the passThroughProps as these props would be all passed to antds AutoComplete which does not accept them.

I know this looks somewhat strange but it's needed.

reverted in 0e724b2

marcjansen · 2017-11-28T22:11:49Z

src/Util/ProjectionUtil/ProjectionUtil.js

+   */
+  static proj4CrsDefinitions() {
+    return {
+      'EPSG:25832': '+proj=utm +zone=32 +ellps=GRS80 +towgs84=0,0,0,0,0,0,0 +units=m +no_defs'


This is not in in the proj database? Or am I misunderstanding stuff here

Is this because we get back the urn-whatever identifiers? is there some way to look these up as well?

marcjansen · 2017-11-28T22:13:01Z

src/Util/ProjectionUtil/ProjectionUtil.js

+  /**
+   * FeatureCollections returned by the GeoServer may be associated with
+   * crs identifiers (e.g. "urn:ogc:def:crs:EPSG::25832") that aren't
+   * supported by proj4 and/or ol3 per default. Add appropriate


no version needed, ol is enough

KaiVolland

Nice new feature @ahennr ! 😍

Some remarks inline and:

I'd prefer the classname CrsCombo just like in the className private.

If you want to stay with CoordinateReferenceSystemCombo plz change the private to react-geo-coordinatereferencesystemcombo

Current pattern should be `react-geo-${this.className.toLowerCase()}` <-- maybe this could really work.

KaiVolland · 2017-11-29T08:07:49Z

src/Field/CoordinateReferenceSystemCombo/CoordinateReferenceSystemCombo.jsx

+     * default: http://epsg.io
+     * @type {String}
+     */
+    epsgIoBaseUrl: PropTypes.string,


The propertyname could be more general like crsApiUrl.

changed in 3cc756f

KaiVolland · 2017-11-29T08:09:08Z

src/Field/CoordinateReferenceSystemCombo/CoordinateReferenceSystemCombo.jsx

+     * CRS) and code (e.g. EPSG-code of CRS) property
+     * @type {Array}
+     */
+    predefinedCrsDefinitions: PropTypes.arrayOf(PropTypes.shape({


KaiVolland · 2017-11-29T08:15:31Z

src/Field/CoordinateReferenceSystemCombo/CoordinateReferenceSystemCombo.jsx

+      q: searchVal
+    };
+
+    fetch(`${epsgIoBaseUrl}?${UrlUtil.objectToRequestString(queryParameters)}`)


I'd prefer to return the fetch result and work with the result where it is called. This keeps this function cleaner. #cohesion

return fetch(`${epsgIoBaseUrl}?${UrlUtil.objectToRequestString(queryParameters)}`) .then(response => response.json());

See below for the changes to handleSearch.

changed in 3cc756f

KaiVolland · 2017-11-29T08:19:19Z

src/Field/CoordinateReferenceSystemCombo/CoordinateReferenceSystemCombo.jsx

+    }
+
+    if (!predefinedCrsDefinitions) {
+      this.fetchCrs(value, crsDefinitions => this.setState({ crsDefinitions, value })) ;


The changes to fetchCrs would lead to further changes here... (see above)

this.fetchCrs(value) .then(this.transformResults) // to be extracted from onFetchSucess .then(crsDefinitions => this.setState({ crsDefinitions, value })) // could be the new onFetchSucces .catch(Logger.error(`Error while requesting in CoordinateReferenceSystemCombo: ${error}`)); // could stay in onFetchError

changed in 3cc756f

KaiVolland · 2017-11-29T08:29:08Z

src/Field/NominatimSearch/NominatimSearch.jsx

-      limit,
-      countrycodes,
-      map,
-      onSelect,


I think this is mistake… plz revert.

KaiVolland · 2017-11-29T08:34:33Z

src/Util/ProjectionUtil/ProjectionUtil.js

+ *
+ * @class
+ */
+export class ProjectionUtil {


This util seems to be projectspecific. If we still want it in react-geo proj4CrsDefinitions and proj4CrsMappings should be removed and instead be passed to the init functions.

Now, crs Mappings and definitions are passed to init functions of util, see 059eaa3

KaiVolland

I'm sry to bother you but one more remark: 💌

KaiVolland · 2017-11-29T10:41:31Z

src/Field/CoordinateReferenceSystemCombo/CoordinateReferenceSystemCombo.jsx

+    if (!predefinedCrsDefinitions) {
+      this.fetchCrs(value)
+        .then(this.transformResults)
+        .then(this.runViaCallback.bind(this, crsDefinitions => this.setState({ crsDefinitions, value })))


I don't get the sense in the runViaCallback method.

.then(this.runViaCallback.bind(this, crsDefinitions => this.setState({ crsDefinitions, value })))

…should be…

.then(crsDefinitions => this.setState({crsDefinitions})) // value is already set in line 149

removed in 645f3d9

KaiVolland · 2017-11-29T13:12:35Z

Great job. Plz merge.

…g-fetch-3.1.0

ahennr added 3 commits November 28, 2017 17:24

adds proj4 as dependency

c70f685

remove uneeded variables in destructuring

0e724b2

adds ProjectionUtil

e49ff3c

ahennr requested review from KaiVolland and marcjansen November 28, 2017 16:32

ahennr mentioned this pull request Nov 28, 2017

Features - RoadMap #240

Closed

56 tasks

ahennr force-pushed the coordinate-reference-system-combo branch from 43f51d2 to afd4333 Compare November 28, 2017 16:44

adds CoordinateReferenceSystemCombo

915ffba

marcjansen approved these changes Nov 28, 2017

View reviewed changes

KaiVolland requested changes Nov 29, 2017

View reviewed changes

ahennr added 4 commits November 29, 2017 10:57

fix classname and removes less file, set width in example

77710ba

revert deletion of destructured variables

e871405

refactoring of epsg io fetch and success / error handling

3cc756f

pass crs Mappings and definitions to init functions in util

059eaa3

ahennr force-pushed the coordinate-reference-system-combo branch from afd4333 to 059eaa3 Compare November 29, 2017 10:12

KaiVolland requested changes Nov 29, 2017

View reviewed changes

removed unneeded method

645f3d9

ahennr force-pushed the coordinate-reference-system-combo branch from 17775ed to 645f3d9 Compare November 29, 2017 12:31

KaiVolland approved these changes Nov 29, 2017

View reviewed changes

ahennr merged commit 190c852 into terrestris:master Nov 29, 2017

ahennr deleted the coordinate-reference-system-combo branch November 29, 2017 13:14

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

Merge pull request #244 from terrestris/dependabot/npm_and_yarn/whatw…

12c9e5a

…g-fetch-3.1.0

Comments

Conversation

ahennr commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcjansen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KaiVolland Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KaiVolland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KaiVolland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahennr Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KaiVolland commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ahennr commented Nov 28, 2017 •

edited

Loading

KaiVolland Nov 29, 2017 •

edited

Loading

KaiVolland left a comment •

edited

Loading

ahennr Nov 29, 2017 •

edited

Loading