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 gmf feature style directive #955

Merged
merged 1 commit into from Apr 4, 2016

Conversation

adube
Copy link
Contributor

@adube adube commented Mar 29, 2016

This PR introduces a gmf-featurestyle directive and an example featuring it. It also introduces a gmf.FeatureHelper service that is responsible of giving utility methods to create, get and set the style for a feature using its inner properties. It also contains methods to calculate and return a measure string from a feature geometry.

Wiki: https://github.com/camptocamp/c2cgeoportal/wiki/Spec-%231658-Measures-and-redlining

Tasks remaining to do

  • Deal with circle measurements, i.e. how should we do it properly. If we do not support ol.geom.Circle as type of geometry, then we need to adjust what has been made. Answer: we'll keep circles as polygons, as before.
  • Travis build pass
  • Code review

Live example

@adube adube changed the title (WIP) Feature tyle (WIP) Feature style directive Mar 29, 2016
@adube adube force-pushed the edit-feature-style branch 4 times, most recently from ca484b9 to ba46128 Compare March 31, 2016 13:06
@adube adube changed the title (WIP) Feature style directive Add gmf feature style directive Mar 31, 2016
@adube
Copy link
Contributor Author

adube commented Mar 31, 2016

@fgravin Ready for review.

@adube adube force-pushed the edit-feature-style branch 2 times, most recently from 0c744cb to f77b562 Compare March 31, 2016 18:26
@adube adube mentioned this pull request Apr 1, 2016
7 tasks
@adube adube changed the title Add gmf feature style directive (WIP) Add gmf feature style directive Apr 1, 2016
@adube adube changed the title (WIP) Add gmf feature style directive Add gmf feature style directive Apr 1, 2016
@pgiraud
Copy link
Contributor

pgiraud commented Apr 4, 2016

@adube Let's forget about the "icon" that was supposed to be displayed ahead of measure. I don't this is required. Can you please remove the bordered text in you example?

if (newFeature) {
keys.push(
ol.events.listen(
newFeature,
Copy link
Member

Choose a reason for hiding this comment

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

Code could be a bit factored.

[gmf.FeatureProperties.ANGLE, gmf.FeatureProperties.COLOR, ...].forEach(function(propName) {
keys.push(
         ol.events.listen(
             newFeature,
             ol.Object.getChangeEventType(propName),
             this.handleFeatureChange_,
             this
         )
     );
}.bind(this)

@fgravin
Copy link
Member

fgravin commented Apr 4, 2016

Looks good in the approach.
Maybe worth it to improve the css (slider etc..), it's gonna be usefull for desktop integration.

Have you watched the FeatureHash format ?
https://github.com/camptocamp/ngeo/blob/master/src/ol-ext/format/featurehash.js
It does parse and serialize styles from/to properties, you need to have a look and be sure everything is coherant and no code is duplicated. Thanks.

@adube
Copy link
Contributor Author

adube commented Apr 4, 2016

  • removal of "icon": OK
  • improve css: OK (for slider, for the rest it's already fine, I think)
  • FeatureHash: I'll need to discuss this more with you, @fgravin.

@adube
Copy link
Contributor Author

adube commented Apr 4, 2016

@fgravin I made all the requested/discussed modifications, with the exception of the modifications to the FeatureHash. It will be easier to make those fixes when we implement the Permalink with it, in an other PR.

Please merge if everything looks okay (once Travis passes).

@adube
Copy link
Contributor Author

adube commented Apr 4, 2016

(You may need to clean your browser cache to see the changes in the live example).

/**
* @enum {string}
*/
ngeo.FeatureProperties = {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn' start with upper case, it's not a class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix this. Sorry, I'm used to OL3 doing it this way, see: https://github.com/openlayers/ol3/blob/master/src/ol/layer/layerbase.js#L12

Copy link
Member

Choose a reason for hiding this comment

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

Well maybe i'm wrong ..

Copy link
Member

Choose a reason for hiding this comment

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

keep it like this then.

Choose a reason for hiding this comment

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

For enums upper-case is fine. 👍

@fgravin fgravin merged commit 69003a2 into camptocamp:master Apr 4, 2016
@adube
Copy link
Contributor Author

adube commented Apr 4, 2016

Thanks for the review.

@adube adube deleted the edit-feature-style branch April 4, 2016 17:57
@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

5 participants