-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ngeo draw feature directive #975
Conversation
@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). |
Looks good for what i've seen so far.
Ok i will think about it. |
You need to pass some html to the constructor options. |
Understood. I'm on it. |
be13c5e
to
29f3abd
Compare
@fgravin This is now ready for review. The demo is up-to-date too. |
controller: 'GmfDrawfeatureController', | ||
scope: { | ||
'active': '=gmfDrawfeatureActive', | ||
'map': '=gmfDrawfeatureMap' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use >
instead (one way binding)
Some minor comments, the overall looks good. |
Thanks for the review. I'll address the changes required. Please, let me know if this should go into ngeo instead. |
What i have thought of in the beginning was a directive draw like this, with the controller logic. Each small directive register itself in the main controller, and adds its specific handlers. What do you think ? |
@fgravin and I discussed about this. It makes sense to move the draw directive in ngeo. I'm on it. |
@fgravin Travis fails. I don't understand why. |
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 ... |
88d39bf
to
07fec4f
Compare
@fgravin Now, each draw/measure directive have its own file. I still have the build issue when making |
9f55ab4
to
d1f0482
Compare
Do a 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. |
bc53c2d
to
451783c
Compare
@fgravin Thanks for the tip. All other examples but mine were throwing an error. That was because of the addition of With this, we're good to go. |
Live example updated. |
* @type {ol.interaction.Draw} | ||
* @export | ||
*/ | ||
this.drawPoint; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove it from here.
Cause the main directive doesn't know what interaction it can have.
Do you agree ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
1e5d5e3
to
111dead
Compare
Can you rebase please ? |
111dead
to
c86fb68
Compare
Done. |
*/ | ||
link: function($scope, element, attrs, drawFeatureCtrl) { | ||
|
||
var helpMsg = gettext('Click to start drawing azimut'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You tell me.
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these informations, I will remove that temporarily.
This PR introduces a
ngeo-drawfeature
directive and an example featuring it.Todo
Live example