Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

add a start to take labels expressions from esri. #146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PieterVanPoyer
Copy link

Hi,

I've made a starting point for adding labels to the esri-leaflet-renderers.
I see this is requested by: ynunokawa/L.esri.WebMap#74 .
I wanted this feature also for my own project.

There is more work to be done in this area. For example only center and topright position is supported now. The expression language is far more complicated than currently implemented.

This is a breaking change. Some users did probably work arround the missing label feature themself.
So a major version bump is required in my opinion.

Kind regards,
Pieter

@jgravois
Copy link
Contributor

related: Esri/esri-leaflet#916

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

very cool! thanks for sharing what you've come up with so far.

Comment on lines +8 to +16
scaleToZoom: function (scaleValue) {
if (scaleValue >= 500000000) {
return 0;
}
if (scaleValue >= 250000000) {
return 1;
}
if (scaleValue >= 150000000) {
return 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

this function could be collapsed with a little more math.
Esri/esri-leaflet#426 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Okay, i'll check that.

@PieterVanPoyer
Copy link
Author

very cool! thanks for sharing what you've come up with so far.

Nice!

I've marked this PR as Work In Progress.
What should I do to finish this pull request?

Some things I am thinking of:

  • Provide an implementation for the other esri label positions (now only top right, and a default to center is provided. [ http://help.arcgis.com/en/sdk/10.0/java_ao_adf/api/arcobjects/com/esri/arcgis/carto/esriServerPointLabelPlacementType.html ]
  • Optimize the check of the zoomlevels range.
  • Write some tests?
  • Provide an example where you can see it work.

What do you think?

@jgravois
Copy link
Contributor

jgravois commented Oct 24, 2019

I don't work at @Esri anymore but that sounds like a good plan to me.

if no one swoops in to help you land your work here, you could always publish it on your own.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants