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

Layerviewcontrol #93

Merged
merged 10 commits into from
Sep 24, 2024
Merged

Conversation

trafficonese
Copy link
Contributor

fixes #90

I didn't add the legend part, I tried a bit but it's painful. And I never add feature layers as basegroup, so it didn't seem a high priority for me.

I also added a shiny example in /inst/test_layerviewcontrol.R

@tim-salabim
Copy link
Member

Thanks, this is awesome!

  • How do you feel about providing an enhanced version of leaflet::addLayersControl() with default behaviour just calling the standard generic? I.e. overriding leaflet::addLayersControl with leafem::addLayersControl when leafem is loaded...
  • For the overlay layers, is it possible to separate the view change logic between show/hide and the home_btn? IMHO the view shouldn't change (by default) when I activate/show an overlay layer, as I might want to simply show/hide with a fixed view to see what changes (especially when I have dedicated buttons for zooming to the layer)

@trafficonese
Copy link
Contributor Author

I would prefer to keep the 2 functions separate or I dont see the advantage of mixing them.

Yes that is definitly possible.

@trafficonese
Copy link
Contributor Author

new argument setviewonselect, which is TRUE by default - so it will zoom on the layer if selected

@tim-salabim
Copy link
Member

I would prefer to keep the 2 functions separate or I dont see the advantage of mixing them.

As it stands currently, addLayerViewControl will not work without a addLayersControl call. So, either we enhance the latter or we would need to change the name, so that it becomes obvious that one is not adding something, but rather updating the existing layers control. updateLayersControl already exists though...

@tim-salabim
Copy link
Member

BTW, I am also contemplating adding options for opacity sliders and color legends. This would make things much cleaner than the current legend solutions that are available...

@trafficonese
Copy link
Contributor Author

I see, you're right. I will look into it.

You mean to inlcude one of those opacity plugins to change the opacity of base and overlay groups, or add a custom JS function?
https://github.com/lizardtechblog/Leaflet.OpacityControls
https://github.com/dayjournal/Leaflet.Control.Opacity
https://github.com/Raruto/leaflet-transparency

And how should the color legend look like? You mean to integrate the whole R-leaflet legend in the layersControl or should it be just 1 colored symbol per group?

@tim-salabim
Copy link
Member

You mean to inlcude one of those opacity plugins to change the opacity of base and overlay groups, or add a custom JS function?

Yes, just a simple horizontal slider that does not take too much space.

And how should the color legend look like?

Just a standard legend, horizontal placed under the layer name, so you'd have

layername | homebutton
horizontal opacity slider
horizontal color legend

Both opacity slider and legend could/should probably be hidden when the layer is hidden...

Legends for standard basemaps are not necessary I guess.

But I can take care of implementing this as soon as we have a working function to extend standard layers control... This is mainly for mapview to make maps look cleaner

@trafficonese
Copy link
Contributor Author

Got it.

We could maybe integrate it in addGroupedLayersControl

Right now I think the layerviewcontrol should work for both leaflet::addLayersControl and leaflet.extras::addGroupedLayersControl, but didnt test it.

@tim-salabim
Copy link
Member

Hm, not a bad idea... I agree that whatever we implement, it should be compatible with both standard and grouped layer control. If I envision all this correctly, then we either need a extendLayersControl function that works with both standard and grouped controls or implement a completely new layers control function that integrates all of this into one function (which could be an extended addGroupedLayersControl I guess... From my point of view, I'd like to have something in leafem that would satisfy my ideas for mapview, that way I have more control... Just thinking out loud

@trafficonese
Copy link
Contributor Author

I went for extendLayerControl and it should be working for normal layercontrols and the grouped version. 🥳

I am hijacking the legends with a corresponding layerId, so the addLegend should have both layerId and group point to the layer group:
addLegend(..., layerId = "mygroup", group = "mygroup")

See the example

image

@tim-salabim
Copy link
Member

tim-salabim commented Sep 10, 2024

Ok, this is fantastic!!! The only small issue I have is that horizontal legends would make it even better :-) Do you think that's possible without too much effort? Package leaflegend does have a solution for it...

@trafficonese
Copy link
Contributor Author

Maybe if we overwrite quite some CSS, but it would be best if addLegend offered this option.

@tim-salabim
Copy link
Member

Maybe we can just utilize leaflegend?
https://roh.engineering/posts/2022/07/leaflegend-recipes/

@trafficonese
Copy link
Contributor Author

I thought the exact same, but I will have to test it as leaflegend is not using a layerId, so the code needs some adapting maybe.

@trafficonese
Copy link
Contributor Author

Gettin there ;)
I just pushed a fix that makes leaflegend work aswell. Full flexibility now 🚀
image

I just noticed, sometimes the layerId needs to be set in leaflegend and sometimes the group was sufficient.

@tim-salabim
Copy link
Member

@trafficonese is this ready to merge?

@trafficonese
Copy link
Contributor Author

yes I think so, allthough I haven't tested it a lot yet.

@tim-salabim tim-salabim merged commit aa87de3 into r-spatial:master Sep 24, 2024
5 checks passed
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.

Adjust setView() for each group
2 participants