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

Set pane in vector feature style to improve ordering #212

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

michaelpnelson
Copy link
Contributor

This improves ordering between layers of type vector/GeoJSON. Multiple GeoJSON layers have features rendered in a shared svg element inside the overlay pane. This amends the styling of features to specify the pane created for the layer. This results in an svg element in each GeoJSON layer pane.

This improves ordering between layers of type vector/GeoJSON. Multiple GeoJSON layers have features rendered in a shared svg element inside the overlay pane. This amends the styling of features to specify the pane created for the layer. This results in an svg element in each GeoJSON layer pane.
@michaelpnelson michaelpnelson self-assigned this Feb 24, 2023
@dgboss
Copy link
Contributor

dgboss commented Feb 27, 2023

Can you provide instructions for reproducing the problem you are trying to solve?

It seems odd to me that setting a pane on the style has an impact on the layer. I thought the pane property was applicable to the layer.options object, not the layer.options.style object. A quick look at the Leaflet code base would seem to support this, but I'm not intimately familiar with the entire code base so perhaps I'm missing something.

@michaelpnelson
Copy link
Contributor Author

Can you provide instructions for reproducing the problem you are trying to solve?

It seems odd to me that setting a pane on the style has an impact on the layer. I thought the pane property was applicable to the layer.options object, not the layer.options.style object. A quick look at the Leaflet code base would seem to support this, but I'm not intimately familiar with the entire code base so perhaps I'm missing something.

The problem is that the ordering of GeoJSON layer features doesn't follow the layer ordering. The earlier layer order change improved ordering between layers of different types. Ordering of layers of the same GeoJSON type seemed OK but we later observed it can differ. We recently changed CleanBC's layer of public charging stations from type Esri Feature to GeoJSON and now the order instability is easy to see (https://cleanbcfleet.apps.gov.bc.ca/) - the public charging station is on top when it should be on the bottom.

'pane' is a parameter in both layer options (https://leafletjs.com/reference.html#geojson-option, inherited from Layer) as well as layer path options (https://leafletjs.com/reference.html#path-option, also inherited from Layer: "By default the layer will be added to the map's overlay pane. Overriding this option will cause the layer to be placed on another pane by default.").

Without this change, all GeoJSON features are drawn in the same svg element in the overlay pane:

Screenshot 2023-02-27 at 9 27 42 AM

With this change applied, each layer gets its own svg element:

Screenshot 2023-02-27 at 9 31 25 AM

So ... I know it looks odd :) but I hope this explains things. I'm happy to demo it for you if you like.

@dgboss
Copy link
Contributor

dgboss commented Feb 27, 2023

Thanks for the comments. I can see what you are trying to solve, but why are you passing the layerPaneId into the convertStyle() function? I don't think pane is a valid property of the style options object being returned by convertStyle() function? Or is it?

@michaelpnelson
Copy link
Contributor Author

Thanks for the comments. I can see what you are trying to solve, but why are you passing the layerPaneId into the convertStyle() function? I don't think pane is a valid property of the style options object being returned by convertStyle() function? Or is it?

I think pane is valid - https://leafletjs.com/reference.html#path-option shows pane as one of the path options inherited from Layer. These path options (such as others we use like "fillColor" and "stroke") aren't CSS options - maybe they're part of a vocabulary unique to Leaflet.

@dgboss
Copy link
Contributor

dgboss commented Feb 27, 2023

Thanks, I missed expanding the properties derived from the layer options! I still find it confusing that you effectively set the pane twice (once via the style and once in layer options around line 160). Would there be any negative consequence to removing pane from layerOptions line 160?

@michaelpnelson
Copy link
Contributor Author

Thanks, I missed expanding the properties derived from the layer options! I still find it confusing that you effectively set the pane twice (once via the style and once in layer options around line 160). Would there be any negative consequence to removing pane from layerOptions line 160?

There doesn't seem to be any impact when I remove pane from layerOptions. I've tested it in multiple SMK apps including one with multiple layers of supported types and haven't noticed any discrepancies in expected behaviour. I'll remove it.

@michaelpnelson michaelpnelson merged commit 4b47699 into master Feb 27, 2023
@michaelpnelson michaelpnelson deleted the vector_layer_order branch February 27, 2023 22:42
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.

2 participants