Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add polygon label placement options #1225

Merged
merged 2 commits into from
May 8, 2022
Merged

Add polygon label placement options #1225

merged 2 commits into from
May 8, 2022

Conversation

beroso
Copy link
Contributor

@beroso beroso commented May 5, 2022

Saw this PR about centered labels for polygons today.

I think it can be useful to add an option to place the label using Dart polylabel. It works well on both convex and concave polygons.

Example:

Centroid (current approach) Polylabel
Screen Shot 2022-05-05 at 17 44 31 Screen Shot 2022-05-05 at 17 44 07

@JaffaKetchup
Copy link
Member

JaffaKetchup commented May 6, 2022

Thanks for your PR! I'll have a proper look tomorrow :)
Just for now though, I can see that the enumerable names might need changing though, as they are a bit confusing. One question, might this have any impact on performance as far as you could see?

@JaffaKetchup JaffaKetchup requested review from johnpryan, JaffaKetchup and ibrierley and removed request for johnpryan May 6, 2022 21:36
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

This is working great! I've commented below on something(s) that might need changing, but I'm open to suggestions :)

Copy link
Contributor

@ibrierley ibrierley left a comment

Choose a reason for hiding this comment

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

I'm fine with this either way. It tests fine. It feels like each option in certain cases is preferable when I look at it visually, so no preference on which is default.

polylabel is maybe ambiguous as an parameter, but I guess it does what it says on the tin (uses whatever polylabel calculates, which could change and not be a center...). So no strong preference either way.

Changed default `labelPlacement` mode to `polylabel`
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM

@JaffaKetchup JaffaKetchup merged commit bf782ba into fleaflet:master May 8, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants