Digitizing tools #377

annarieger · 2018-01-30T14:27:16Z

FEATURE

Description:

This MR introduces the basic digitizing tools to draw, select, modify and delete vector features on/from the map.

The following tools are supported at the moment:

draw point
draw linestring
draw polygon
draw labeled point
draw circle
draw polygon
select and modify feature
select and delete feature
select and copy feature

Please review @terrestris/devs

…s and polygons with default non customizable style

marcjansen

Very nice work so far. Looking forward to checking this again once you consider it no longer WIP.

marcjansen · 2018-01-30T14:36:19Z

src/Button/RedliningButton/RedliningButton.example.md

+---
+layout: basic.hbs
+title: RedliningButton example
+description: This is a example showing an RedliningButton.


…many redlining buttons

Changed in 97f906e

marcjansen · 2018-01-31T06:55:08Z

src/Button/RedliningButton/RedliningButton.spec.jsx

+    });
+  });
+
+  describe('#Static methods', () => {


These aren't statics, are they?

No, they're not, fixed in 3a0a656

marcjansen · 2018-01-31T06:57:43Z

src/Button/RedliningButton/RedliningButton.spec.jsx

+
+        const mockInteraction = getMockDrawPointInteraction();
+
+        const defaultMapInteractionsLength = map.getInteractions().getArray().length;


Or map.getInteractions().getLength();

fixed in 3a0a656

marcjansen · 2018-01-31T06:57:57Z

src/Button/RedliningButton/RedliningButton.spec.jsx

+          interaction: [mockInteraction]
+        });
+
+        expect(map.getInteractions().getArray().length).toBe(defaultMapInteractionsLength + 1);


fixed in 3a0a656

marcjansen · 2018-01-31T07:00:40Z

src/Button/RedliningButton/RedliningButton.spec.jsx

+        map.addInteraction(mockInteraction);
+        instance._redliningFeatures.on('add', wrapper.instance().handleTextAdding);
+
+        expect(instance._redliningFeatures.listeners_.add.length).toBe(2);


This and the lines below where we use the private property .listeners_ are a bit fragile. Right now I don't have an alternative, but at least a comment would be nice.

comment added in 3a0a656

marcjansen · 2018-01-31T07:01:42Z

src/Button/RedliningButton/RedliningButton.spec.jsx

+
+        instance.onToggle(false);
+
+        expect(instance._selectInteraction.listeners_select).toBeUndefined();


Should this bbe instance._selectInteraction.listeners.select_? Is this just stubbed right now?

oh, nice catch.. fixed in 3a0a656

marcjansen · 2018-01-31T07:15:08Z

src/Button/RedliningButton/RedliningButton.spec.jsx

+
+        instance.onToggle(false);
+
+        expect(instance._selectInteraction.listeners_select).toBeUndefined();


Same question.

one another nice catch :) fixed in 3a0a656

marcjansen · 2018-01-31T07:16:43Z

src/Util/DigitizeUtil/DigitizeUtil.js

+ *
+ * @class DigitizeUtil
+ */
+class DigitizeUtil {


Would a more appropriate name be FeatureUtil?

Renamed to AnimateUtil in 0504b8e. I think it's even more meaningful as FeatureUtil

marcjansen · 2018-01-31T07:18:21Z

src/Util/DigitizeUtil/DigitizeUtil.js

+         vectorContext.setFillStrokeStyle(
+           style.getFill(), style.getStroke());
+         vectorContext.setImageStyle(style.getImage());
+         if (geometry instanceof ol.geom.Point) {


Here and below:

Should we import the ol/geom/Point and use thes in the instanceof checks?

Yeah, indeed! And could you please remove the ol entry from the eslint globals (see .eslintrc)?

.eslintrc adapted in 4a17d2a, imports fixed in 4a1ea1e

marcjansen · 2018-01-31T07:36:26Z

I have adressed some minor comments in annarieger#1, you decide if you want to have these.

Minor redlining enhancements

dnlkoch · 2018-01-31T08:22:33Z

src/Button/RedliningButton/RedliningButton.jsx

+ * @extends React.Component
+ *
+ */
+class RedliningButton extends React.Component {


Not technical, but could we please get rid of the term Redlining (see here).

Since 938d625 the class is renamed to DigitizeButton

dnlkoch · 2018-01-31T08:27:40Z

src/Button/RedliningButton/RedliningButton.jsx

+        this.createDrawInteraction(pressed);
+      } else if (editType) {
+        this.createSelectOrModifyInteraction(pressed);
+


Minor: Empty line.

fixed in 08a4ce5

dnlkoch · 2018-01-31T08:29:55Z

src/Button/RedliningButton/RedliningButton.jsx

+
+    this.state = {
+      redliningLayer: null,
+      interaction: null,


Name is misleading as it contains an array of interactions.

You're so right, renamed it in d9df6bd

dnlkoch · 2018-01-31T08:35:16Z

src/Button/RedliningButton/RedliningButton.jsx

+          }),
+          stroke: new OlStyleStroke({
+            color: !useSelectStyle ? strokeColor : selectStrokeColor,
+            width: 2


This and some other style properties aren't configurable yet. Do you have any attempt to make them configurable, e.g. by passing an ol.Style for each draw type as prop?

yeah, please make these cinfigurable

I've opened an issue -> #380

Will be done in one of follow-ups 😇

dnlkoch · 2018-01-31T08:36:22Z

src/Button/RedliningButton/RedliningButton.jsx

+    let geometryFunction;
+    let type = drawType;
+
+    if (drawType === 'Rectangle') {


Minor: Please move all draw and edit type strings to statics.

fixed in df4e3fd

dnlkoch · 2018-01-31T08:38:54Z

src/Button/RedliningButton/RedliningButton.jsx

+   * Creates a correctly configured OL select and/or modify and/or translate
+   * interaction(s) depending on given editType and adds this/these to the map.
+   *
+   * @method


Just curious, but is the @method tag needed here for jsdoc?

dnlkoch · 2018-01-31T08:39:18Z

src/Button/RedliningButton/RedliningButton.jsx

+    this._selectInteraction = new OlInteractionSelect({
+      condition: OlEventsCondition.singleClick,
+      style: this.getRedliningStyleFunction,
+      hitTolerance: 5


Minor: Should be configurable.

I've opened an issue -> #380

Will be done in one of follow-ups 😇

dnlkoch · 2018-01-31T08:40:27Z

src/Button/RedliningButton/RedliningButton.jsx

+        features: this._selectInteraction.getFeatures(),
+        deleteCondition: OlEventsCondition.singleClick,
+        style: this.getRedliningStyleFunction,
+        pixelTolerance: 10


Minor: The default value is not in sync with the hitTolerance of the select interaction and should be configurable as well.

I've opened an issue -> #380

Will be done in one of follow-ups 😇

dnlkoch · 2018-01-31T08:42:24Z

src/Button/RedliningButton/RedliningButton.jsx

+    map.on('pointermove', this.onPointerMove);
+
+    this.setState({
+      interaction: interactions


Is there a need for checking the pressed state of the button in callback (like in the createDrawInteraction())?

No, no check needed here

dnlkoch · 2018-01-31T08:49:43Z

src/Button/RedliningButton/RedliningButton.jsx

+    const feat = evt.selected[0];
+    const copy = feat.clone();
+    //eslint-disable-next-line
+    const doneFn = finalFeature => {this._redliningFeatures.push(finalFeature)};


Absolutely minor: Missing colon behind this._redliningFeatures.push(finalFeature).

fixed in 08a4ce5

dnlkoch · 2018-01-31T08:54:33Z

src/Button/RedliningButton/RedliningButton.jsx

+    this.setState({
+      textLabel: ''
+    });
+


Minor: Empty line.

fixed in 08a4ce5

dnlkoch · 2018-01-31T08:56:01Z

src/Button/RedliningButton/RedliningButton.jsx

+   *
+   * @method
+   */
+  setTextOnFeature = (feat) => {


Parentheses can be omitted.

fixed in 08a4ce5

dnlkoch · 2018-01-31T09:01:54Z

src/Button/RedliningButton/RedliningButton.jsx

+
+    const { map } = this.props;
+    const pixel = map.getEventPixel(evt.originalEvent);
+    const hit = map.hasFeatureAtPixel(pixel);


Unsure here, but we may want to include a (customizable) layerFilter to only change the cursor for features/layers available to edit with the given interactions.

I've opened an issue -> #380

Will be done in one of follow-ups 😇

dnlkoch · 2018-01-31T09:18:32Z

src/Button/RedliningButton/RedliningButton.jsx

+      : this.className;
+
+    return (
+      <div className="wrapper">


Did you try to set the render children as an array like:

return([ <ToggleButton>, <Modal> <Input /> </Modal> ]);

This way it might be possible to avoid the usage of the wrapper div.

We've tried it out together with @KaiVolland and we think it would be better to leave this without changes here

dnlkoch · 2018-01-31T09:21:03Z

src/Button/RedliningButton/RedliningButton.less

+  }
+}
+
+.wrapper {


If the wrapper can't be avoided, please rename to react-geo-redlining-button-wrapper or similar.

done in 08a4ce5

dnlkoch · 2018-01-31T09:23:38Z

src/Util/DigitizeUtil/DigitizeUtil.js

+   * in the end with given `duration` in ms, using the given style,
+   * calling a `doneFn`.
+   *
+   * @param {OlMap} map An OlMap.


OlMap -> ol.Map

To get in sync with the ol API and to harmonize with the method documentation overall. But it's still non-blocking…

KaiVolland

Very nice work.

Added some remarks you might want to tackle.

I think we should get rid of the name redlining as @dnlkoch mentioned.

KaiVolland · 2018-01-30T14:39:33Z

src/Button/RedliningButton/RedliningButton.jsx

+   * @type {OlFeature}
+   * @private
+   */
+   _redliningTextFeature = null;


Is the indention correct? Hard to see in the diff view but it seems like 3 instead of 2 spaces…

fixed in 08a4ce5

KaiVolland · 2018-01-30T14:49:23Z

src/Button/RedliningButton/RedliningButton.jsx

+
+    super(props);
+
+    if (!this.props.drawType && !this.props.editType) {


You may wan't to use a custom proptype validator instead: https://reactjs.org/docs/typechecking-with-proptypes.html

I'd prefer to leave this check as is cause of using of oneOf check for both props drawType and editType

KaiVolland · 2018-01-31T08:41:29Z

src/Button/RedliningButton/RedliningButton.jsx

+   *
+   * @method
+   */
+  getRedliningStyleFunction = feature => {


Isn't it needed that we can pass the style(s) from outside as a prop?

I've opened an issue -> #380

Will be done in one of follow-ups 😇

KaiVolland · 2018-01-31T08:50:21Z

src/Button/RedliningButton/RedliningButton.jsx

+  onFeatureCopy = evt => {
+    const feat = evt.selected[0];
+    const copy = feat.clone();
+    //eslint-disable-next-line


Why do you disable eslint here?

Because of using of inline function in the next line which needs JS Doc comment

KaiVolland · 2018-01-31T08:58:43Z

src/Button/RedliningButton/RedliningButton.jsx

+    const pixel = map.getEventPixel(evt.originalEvent);
+    const hit = map.hasFeatureAtPixel(pixel);
+
+    map.getTargetElement().style.cursor = hit ? 'pointer' : '';


Very minor: cursor could be move (or grab and grabbing) for move interaction. Maybe there are some more semantic cursors for the different controls.
Compare: https://developer.mozilla.org/de/docs/Web/CSS/cursor

I think it's no needed cause ol.interaction.Modify and ol.interaction.Translate already provides the appropriate cursors as soon as interaction was activated

KaiVolland · 2018-01-31T09:00:35Z

src/Button/RedliningButton/RedliningButton.spec.jsx

+import OlGeomLineString from 'ol/geom/linestring';
+import OlGeomPolygon from 'ol/geom/polygon';
+// import OlOverlay from 'ol/overlay';
+// import OlObservable from 'ol/observable';


Plz remove if not used.

removed in 3a0a656

KaiVolland · 2018-01-31T09:11:20Z

src/Util/DigitizeUtil/DigitizeUtil.js

+     const deltaX = totalDisplacement / expectedFrames;
+     const deltaY = totalDisplacement / expectedFrames;
+
+     //eslint-disable-next-line


Why do you disable es lint here?

KaiVolland · 2018-01-31T09:14:29Z

src/Util/DigitizeUtil/DigitizeUtil.js

+   *
+   * @return {String} A listener key from a postcompose event.
+   */
+   static moveFeature = (map, featureToMove, duration, pixel, style, doneFn) => {


I'd prefer to return a Promise instead of passing a callback function.

Changed in 0504b8e

marcjansen · 2018-02-01T10:28:29Z

src/Button/DigitizeButton/DigitizeButton.example.md

+---
+layout: basic.hbs
+title: DigitizeButton example
+description: This is a example showing digitize buttons with different interactions.


Thanks for reviews guys 🍍

Everything is fixed, I'll merge now

marcjansen · 2018-02-01T17:30:18Z

Nice work, everyone. Thanks

…/cli-7.12.17

annarieger added 11 commits January 23, 2018 13:39

introduced RedliningButton class, implemented drawing of points, line…

3cbb491

…s and polygons with default non customizable style

support drawing of circles and rectangles as well

ec986ee

added examples for redlining of simple shapes

6edb9d9

implemented drawing of labeled features, introduced style function

b343f81

added stringDivider method to wrap long labels

05792dd

implemented selecting, modifying and translating of redlining features

7e0b592

implement removing of redlining features

f2107d0

renamed some props

3fd4d5e

implemented copying/cloning of drawn features

44d3dc2

added examples

dfc1a1d

added tests for RedliningButton class

91ca699

marcjansen reviewed Jan 31, 2018

View reviewed changes

marcjansen added 2 commits January 31, 2018 08:24

Add redlining button example to webpack config

258ebf7

Slightly reword example description

bab240f

Merge pull request #1 from marcjansen/annarieger/redlining-tools

7d37888

Minor redlining enhancements

dnlkoch reviewed Jan 31, 2018

View reviewed changes

KaiVolland requested changes Jan 31, 2018

View reviewed changes

KaiVolland added the feature label Jan 31, 2018

annarieger added 7 commits January 31, 2018 15:28

provided further tests

3a0a656

renamed redlining to digitize

938d625

adapted example description

97f906e

removed entry for ol from globals

4a17d2a

fixed OL imports

4a1ea1e

renamed interaction to interactions

d9df6bd

moved drawType and editType names to statics

df4e3fd

marcjansen reviewed Feb 1, 2018

View reviewed changes

annarieger added 3 commits February 1, 2018 11:45

some minors

08a4ce5

fix typo

87a5cd8

renamed DigitizeUtil to AnimateUtil

0504b8e

KaiVolland approved these changes Feb 1, 2018

View reviewed changes

annarieger changed the title ~~WIP Redlining tools~~ Digitize tools Feb 1, 2018

annarieger merged commit 29c4443 into terrestris:master Feb 1, 2018

annarieger changed the title ~~Digitize tools~~ Digitizing tools Feb 1, 2018

annarieger deleted the redlining-tools branch February 1, 2018 14:11

annarieger mentioned this pull request Feb 2, 2018

Digitize tools enhancements #381

Merged

KaiVolland mentioned this pull request Feb 2, 2018

Features - RoadMap #240

Closed

56 tasks

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

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

b97b60c

…/cli-7.12.17


		const mockInteraction = getMockDrawPointInteraction();

		const defaultMapInteractionsLength = map.getInteractions().getArray().length;


		instance.onToggle(false);

		expect(instance._selectInteraction.listeners_select).toBeUndefined();


		super(props);

		if (!this.props.drawType && !this.props.editType) {

Digitizing tools #377

Digitizing tools #377

Conversation

annarieger commented Jan 30, 2018 • edited

FEATURE

Description:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcjansen commented Jan 31, 2018

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

dnlkoch Jan 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnlkoch Jan 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annarieger Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annarieger Feb 1, 2018 • edited

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

dnlkoch Jan 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnlkoch Jan 31, 2018 • edited

Choose a reason for hiding this comment

annarieger Feb 1, 2018 • edited

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 left a comment • edited

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

annarieger commented Jan 30, 2018 •

edited

dnlkoch Jan 31, 2018 •

edited

dnlkoch Jan 31, 2018 •

edited

annarieger Feb 1, 2018 •

edited

annarieger Feb 1, 2018 •

edited

dnlkoch Jan 31, 2018 •

edited

dnlkoch Jan 31, 2018 •

edited

annarieger Feb 1, 2018 •

edited

KaiVolland left a comment •

edited