Skip to content

Fix printing circles#49

Open
maltaesousa wants to merge 5 commits intogeoblocks:mainfrom
maltaesousa:fix-circles
Open

Fix printing circles#49
maltaesousa wants to merge 5 commits intogeoblocks:mainfrom
maltaesousa:fix-circles

Conversation

@maltaesousa
Copy link

This PR fixes an issue where a Circle was converted to a FeatureCollection and not printed at all.

You can reproduce the bug here:

These modifications allow to keep the original style of the circle and just convert the geometry

@maltaesousa
Copy link
Author

ping @gberaudo or @ger-benjamin :) Sorry I can't ask for reviews

Co-authored-by: Stéphane Brunner <stephane.brunner@camptocamp.com>
@maltaesousa
Copy link
Author

hello, any updates? :)

Copy link
Member

@ger-benjamin ger-benjamin left a comment

Choose a reason for hiding this comment

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

Sorry, I don't really have time for that currently :-/
It looks fine to me, but is it fine to NOT do anymore (was line 171):

feature = this.featureCircleAsPolygon(feature as Feature<Circle>);

And after that, still doing ?:

const origGeojsonFeature = this.geojsonFormat.writeFeatureObject(feature);

Also featureCircleAsPolygon is now unused, I think e can remove it (but it's protected, that means, that's a breaking change to remove it)

@maltaesousa
Copy link
Author

@ger-benjamin Fixed what you've mentioned and added a test for what was happening on geogirafe. Let me know if anything is missing or unclear

{
"name": "@geoblocks/mapfishprint",
"version": "0.2.22",
"version": "0.2.23",
Copy link
Member

Choose a reason for hiding this comment

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

@gberaudo a breaking change is a major version, right ? We shoule start to follow strict semver rules.

Suggested change
"version": "0.2.23",
"version": "1.0.0",

I don't care to make a v1 on such bugfix. That's just a number.

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.

3 participants