Add ngeo draw feature directive #975

adube · 2016-04-01T15:04:53Z

This PR introduces a ngeo-drawfeature directive and an example featuring it.

Todo

deal with i18n values
use contants for feature properties and geometry types once Add gmf feature style directive #955 is merged
set the default style of the features when drawn once Add gmf feature style directive #955 is merged
prevent the features and overlay that are added in the measure interactions to be shown, as those conflict with the actual drawn features.
code review
merge into one commit
travis build passes

Live example

http://adube.github.io/ngeo/edit-feature-draw/examples/drawfeature.html

adube · 2016-04-01T15:07:59Z

@fgravin The directive currently contains lots of copy/pasted code from many places: Luxembourg, ngeo examples, etc. The i18n text contained within the directive are a result of that. Would you please tell me what strategy I should use for the i18n ?

If you want to review what's in progress, go ahead. Keep in mind what's remaining to do (see in the description).

fgravin · 2016-04-04T09:12:11Z

Looks good for what i've seen so far.

Would you please tell me what strategy I should use for the i18n ?

Ok i will think about it.

fgravin · 2016-04-04T14:21:27Z

The i18n text contained within the directive are a result of that. Would you please tell me what strategy I should use for the i18n

See https://github.com/Geoportail-Luxembourg/geoportailv3/blob/30272dc5426b6c2d60d7a9aa8d5d38843ca93360/geoportailv3/static/js/measure/measuredirective.js#L138

You need to pass some html to the constructor options.
This html must be compiled ($compile) with the directive translate, this way the string will be replaced on the fly if the language change.

adube · 2016-04-04T15:04:35Z

Understood. I'm on it.

adube · 2016-04-04T19:44:12Z

@fgravin This is now ready for review. The demo is up-to-date too.

fgravin · 2016-04-05T11:47:08Z

contribs/gmf/src/directives/drawfeature.js

+    controller: 'GmfDrawfeatureController',
+    scope: {
+      'active': '=gmfDrawfeatureActive',
+      'map': '=gmfDrawfeatureMap'


use > instead (one way binding)

fgravin · 2016-04-05T12:09:44Z

Some minor comments, the overall looks good.
But i wonder if this shouldn't go into ngeo in a more customisable fashion.

adube · 2016-04-05T12:20:15Z

Thanks for the review. I'll address the changes required.

Please, let me know if this should go into ngeo instead.

fgravin · 2016-04-05T12:32:41Z

What i have thought of in the beginning was a directive draw like this, with the controller logic.
Then, one small directive per drawing type, requiring the draw directive, accessing its controller.

Each small directive register itself in the main controller, and adds its specific handlers.
Common handlers and whole logic would be in main draw directive, no template for this one.
It would go into ngeo.

What do you think ?

adube · 2016-04-05T12:58:49Z

@fgravin and I discussed about this. It makes sense to move the draw directive in ngeo. I'm on it.

adube · 2016-04-05T18:31:03Z

@fgravin Travis fails. I don't understand why.

sbrunner · 2016-04-06T06:52:32Z

You see that the error is in the animation example ... https://travis-ci.org/camptocamp/ngeo/jobs/120962154#L546, I also don't understand why it failed ...

adube · 2016-04-06T17:54:44Z

@fgravin Now, each draw/measure directive have its own file.

I still have the build issue when making make check-examples (see Travis build error). I still don't know what's going on wrong. That prevents me from updating the live example. If you have a minute to check what's wrong I'd appreciate it.

fgravin · 2016-04-07T11:50:08Z

Do a make hosted-exampleand test your example in compiled mode (localhost hosted-examples ..).
You'll see an error, with a link, open the link if not to long, it can give help.

Otherwise, try to remove your subdirective from the template, see if you still have the error. Try to debug step by step by removing some code to see what's going on.
You don't miss any @ngInject ?

adube · 2016-04-07T14:46:00Z

@fgravin Thanks for the tip. All other examples but mine were throwing an error. That was because of the addition of gettext as dependency in ngeo. Now, all examples properly include the .js file needed.

With this, we're good to go.

adube · 2016-04-07T14:47:48Z

Live example updated.

fgravin · 2016-04-07T15:16:24Z

src/directives/drawfeature.js

+   * @type {ol.interaction.Draw}
+   * @export
+   */
+  this.drawPoint;


I think we should remove it from here.
Cause the main directive doesn't know what interaction it can have.
Do you agree ?

Mmm... I liked having those listed here. Those 6 draw/measure directives can only be used within this main drawFeature directive, and those properties identify them correctly. If one looks at this file only, one will understand the different interactions that can be bound from here.

I'm not against removing them. I just find it useful. Also, I haven't tested but this makes the properties correctly exported by closure, I think. Otherwise, we would probably need to do drawFeatureCtrl['drawPoint'] = drawPoint.

Bottom line: I'm in favour of keeping them. What do you think ?

fgravin · 2016-04-11T10:46:29Z

Can you rebase please ?

adube · 2016-04-11T11:54:44Z

Done.

fgravin · 2016-04-11T12:13:09Z

src/directives/measureazimut.js

+     */
+    link: function($scope, element, attrs, drawFeatureCtrl) {
+
+      var helpMsg = gettext('Click to start drawing azimut');


I don't really get it, i know it comes from Lux.
What is helpMsg value, the translation of the key ?

Does translate directive below is usefull ? I think something is redundant here.

You tell me.

ger-benjamin · 2016-04-27T10:52:43Z

contribs/gmf/src/directives/partials/search.html

@@ -8,4 +8,5 @@
 <span class="clear-button ng-hide"
        ng-hide="!ctrl.clearButton || ctrl.input_value == ''"
        ng-click="ctrl.onClearButton()"></span>
+  <input data-ng-model="color" data-ng-change="ctrl.setStyleColor(color)"></input>


@adube Can you please explain me what is that ? That creates a field below the search field that seems not used.
Is that an error ? What's the aim ?

Maybe related to this https://github.com/camptocamp/c2cgeoportal/wiki/Specs-%23869---Color-of-search-result

This was part of a patch that @fgravin did for me. I hadn't noticed it, but it shouldn't have been added. You can remove this.

Thanks for these informations, I will remove that temporarily.

adube changed the title ~~(WIP) Draw Feature Directive~~ (WIP) gmf draw feature directive Apr 1, 2016

adube force-pushed the edit-feature-draw branch from 1b0e5a1 to 0f59e31 Compare April 4, 2016 14:01

adube force-pushed the edit-feature-draw branch 3 times, most recently from be13c5e to 29f3abd Compare April 4, 2016 19:38

adube changed the title ~~(WIP) gmf draw feature directive~~ Add gmf draw feature directive Apr 4, 2016

fgravin reviewed Apr 5, 2016
View reviewed changes

adube force-pushed the edit-feature-draw branch from 29f3abd to f3c7354 Compare April 5, 2016 12:32

adube changed the title ~~Add gmf draw feature directive~~ Add ngeo draw feature directive Apr 5, 2016

adube force-pushed the edit-feature-draw branch from 0c8e267 to e40bf2d Compare April 5, 2016 18:17

adube force-pushed the edit-feature-draw branch 3 times, most recently from 88d39bf to 07fec4f Compare April 6, 2016 17:50

adube force-pushed the edit-feature-draw branch 2 times, most recently from 9f55ab4 to d1f0482 Compare April 7, 2016 11:37

adube force-pushed the edit-feature-draw branch 2 times, most recently from bc53c2d to 451783c Compare April 7, 2016 14:43

fgravin reviewed Apr 7, 2016
View reviewed changes

adube force-pushed the edit-feature-draw branch 3 times, most recently from 1e5d5e3 to 111dead Compare April 8, 2016 15:59

Add ngeo-drawfeature directive and example

c86fb68

adube force-pushed the edit-feature-draw branch from 111dead to c86fb68 Compare April 11, 2016 11:54

fgravin reviewed Apr 11, 2016
View reviewed changes

fgravin merged commit 5496258 into camptocamp:master Apr 11, 2016

ger-benjamin reviewed Apr 27, 2016
View reviewed changes

adube deleted the edit-feature-draw branch October 17, 2016 12:55

sbrunner added this to the Older milestone Aug 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ngeo draw feature directive #975

Add ngeo draw feature directive #975

adube commented Apr 1, 2016

adube commented Apr 1, 2016

fgravin commented Apr 4, 2016

fgravin commented Apr 4, 2016

adube commented Apr 4, 2016

adube commented Apr 4, 2016

fgravin Apr 5, 2016

fgravin commented Apr 5, 2016

adube commented Apr 5, 2016

fgravin commented Apr 5, 2016

adube commented Apr 5, 2016

adube commented Apr 5, 2016

sbrunner commented Apr 6, 2016

adube commented Apr 6, 2016

fgravin commented Apr 7, 2016

adube commented Apr 7, 2016

adube commented Apr 7, 2016

fgravin Apr 7, 2016

adube Apr 7, 2016

fgravin commented Apr 11, 2016

adube commented Apr 11, 2016

fgravin Apr 11, 2016

adube Apr 11, 2016

ger-benjamin Apr 27, 2016 •

edited

pgiraud Apr 27, 2016

adube Apr 27, 2016

ger-benjamin Apr 27, 2016 •

edited

Add ngeo draw feature directive #975

Add ngeo draw feature directive #975

Conversation

adube commented Apr 1, 2016

Todo

Live example

adube commented Apr 1, 2016

fgravin commented Apr 4, 2016

fgravin commented Apr 4, 2016

adube commented Apr 4, 2016

adube commented Apr 4, 2016

fgravin Apr 5, 2016

Choose a reason for hiding this comment

fgravin commented Apr 5, 2016

adube commented Apr 5, 2016

fgravin commented Apr 5, 2016

adube commented Apr 5, 2016

adube commented Apr 5, 2016

sbrunner commented Apr 6, 2016

adube commented Apr 6, 2016

fgravin commented Apr 7, 2016

adube commented Apr 7, 2016

adube commented Apr 7, 2016

fgravin Apr 7, 2016

Choose a reason for hiding this comment

adube Apr 7, 2016

Choose a reason for hiding this comment

fgravin commented Apr 11, 2016

adube commented Apr 11, 2016

fgravin Apr 11, 2016

Choose a reason for hiding this comment

adube Apr 11, 2016

Choose a reason for hiding this comment

ger-benjamin Apr 27, 2016 • edited

Choose a reason for hiding this comment

pgiraud Apr 27, 2016

Choose a reason for hiding this comment

adube Apr 27, 2016

Choose a reason for hiding this comment

ger-benjamin Apr 27, 2016 • edited

Choose a reason for hiding this comment

ger-benjamin Apr 27, 2016 •

edited

ger-benjamin Apr 27, 2016 •

edited