Skip to content
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 the modify circle interaction. #986

Merged
merged 8 commits into from Apr 13, 2016
Merged

Conversation

jlap
Copy link
Contributor

@jlap jlap commented Apr 5, 2016

This commit adds the modify circle interaction, along with an example of how it works.

The interaction was taken from the luxembourg app.

A live example is available here: http://adube.github.io/ngeo/modify-circle/examples/modifycircle.html

@jlap
Copy link
Contributor Author

jlap commented Apr 5, 2016

@fgravin Could you take a look at my travis error? I don't know how to fix it.

@fgravin
Copy link
Member

fgravin commented Apr 11, 2016

The message is explicit, it doesn't find your constructor in build mode. It is probably because you haven't exported your class, so the name are minified on compiling.

  1. Add @export in your constructor api doc
  2. You need to define a type for your interaction options, see ngeox.interaction.MeasureOptions in ngeox.js

@jlap
Copy link
Contributor Author

jlap commented Apr 13, 2016

Thanks @fgravin , the missing export was the problem. I now have a different build error, that only occurs on travis. Building the example locally works for me now (it didn't with the last error), but travis now throws a very cryptic error message, do you have an idea what it means?

Thanks for the help!

@fgravin
Copy link
Member

fgravin commented Apr 13, 2016

Building the example locally works

I'm perplex :)
Add the gettext.js file in your example.

<script src="../node_modules/angular-gettext/dist/angular-gettext.js"></script>

Tell me if it fixes it.

@jlap
Copy link
Contributor Author

jlap commented Apr 13, 2016

That worked thanks!

Here's a live example: http://adube.github.io/ngeo/modify-circle/examples/modifycircle.html

@jlap jlap changed the title [wip] Add the modify circle interaction. Add the modify circle interaction. Apr 13, 2016
@jlap
Copy link
Contributor Author

jlap commented Apr 13, 2016

@fgravin This PR is now ready for review

@sbrunner
Copy link
Member

The live example looks good :-)

* wrapX: (boolean|undefined)}}
* @api
*/
ngeox.interaction.ModifyOptions;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be ModifyCircleOptions, there gonna be other modify interactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not specific to the circle modification interation though, it's actually just about the same as the olx.interaction.ModifyOptions, and I think all modify interactions which inherit from ol's modify interactions will need these options.

(Actually I don't know why just using olx.interaction.ModifyOptions wasn't working, since it's really all I needed)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so maybe it was enough with olx.interaction.ModifyOptions ??
This issue you got was just from the @export thing, maybe you should revert this then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I'm trying it now, I think you're right. I had done it in another commit after the export one because I still had an error, but that must have been the missing ngettext error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so update your PR please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is updated, it passed the travis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i may have mixed something, weren't we talking about removing the type ngeox.interaction.ModifyOptions to use the type olx.interaction.ModifyOptions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we were. We're currently in a comment thread in an outdated commit of the diff, if you go to https://github.com/camptocamp/ngeo/pull/986/files it'll be up-to-date.

I'll squash the commits of the PR into a single one once you say this is ready to merge.

@fgravin
Copy link
Member

fgravin commented Apr 13, 2016

One comment that you have to change please.
Other wise looks good thanks @jlap

@fgravin fgravin merged commit 789ed82 into camptocamp:master Apr 13, 2016
@adube adube deleted the modify-circle branch October 17, 2016 12:54
@sbrunner sbrunner added this to the Older milestone Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants