-
Notifications
You must be signed in to change notification settings - Fork 32
Fix labels/border/remove/clear-instances, **Src-functions, etc.. #95
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
Conversation
…ivity, hoverWait, etc), expose mouseover to Shiny, dont reassign ...
…d "clearGlGroup", set active = TRUE/FALSE when hiding/showing, refactor docs
|
If robertleeplummerjr/Leaflet.glify#153 gets merged the |
…sts for popup = FALSE
|
Here's an example Shiny-App with some street data of Vienna. You can see that the click events (leaflet and leafgl) dont return the same thing and Glify is way off now sometimes and some elements are not selectable at all anymore. Details
|
|
The PR robertleeplummerjr/Leaflet.glify#157 should fix the click/hover on Lines. |
|
Ok, thanks for all the work on this. I'll test when I get back from a short vacation at the end of the week. |
|
@trafficonese I am about to merge this one. Is that ok? Or will it break things somehow? |
|
Yes this will currently break things, as it includes the new Leaflet.Glify version. You would have to also inlcude robertleeplummerjr/Leaflet.glify#157, since it fixes the hover and click events. With 157 merged, I think this PR should be ready to go, but it's been a while, so I would definitly run some tests. And as a reminder:
|
|
Ok, then I think I'm holding off on this one for now. Let's wait until the upstream PR gets merged. |
|
robertleeplummerjr/Leaflet.glify#157 got merged. |
|
Sounds like a good idea. |
Merge branch 'fixlabels' of https://github.com/trafficonese/leafgl into fixlabels # Conflicts: # DESCRIPTION
|
Yes, more or less:
and I think the layers are redrawn on zoom now aswell.. (nope, thats still on the beta branch) robertleeplummerjr/Leaflet.glify@0bd9217 |
|
Good to go in your opinion @trafficonese ? Happy to test, especially as it will fix an issue I'm hitting by the looks: r-tmap/tmap#1064 |
Robinlovelace
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an epic PR! Happy to test if needs be...
| jsonify, | ||
| leaflet, | ||
| sf, | ||
| yyjsonr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should make things faster.
| @@ -1,8 +1,32 @@ | |||
| leafgl development-version | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
@Robinlovelace Yes, please feel free to install the PR and test it. |
|
I am not sure if I had already included the latest Leaflet.Glify version, So i added it again. |
|
Apologies for not getting round to testing it. Would that still be useful @trafficonese and @tim-salabim ? |
|
@Robinlovelace yes, definitly! |
|
@tim-salabim and @Robinlovelace I would merge this PR if that's ok with you? |
|
Yes, please go ahead. Thank you! |
This PR ended up being quite substantial (initially, I just wanted to fix the labels, but I got carried away). Should I split the commits into multiple PRs?
I fixed the **Src frunctions but in my benchmarks they were always slower than the original version. Also since the **Src functions cannot be called directly anymore, I rearranged the code to not test the data/group etc twice.
I found a new JSON converter yyjsonr which appears to be much faster than jsonify (benchmarks are in ther Readme). For now it is used for all the color/label/weight/popup conversion and the point-data. It can also transform Geojson, but not in the structure expected by Leaflet.Glify. The whole
{"type":"FeatureCollection","features":overhead is missing. I tried to add the GeoJson overhead in the functionyyjsonr_2_geojson, but that doesnt seem to be faster thangeojsonsf::sf_geojson, so I commented it out.Also in the last commit I harcoded
hoverwait: 10. Exposing this option doesn't make sense because it is a global argument. The first leafgl-layer that sets this value does so globally, and omitting it defaults to 250, which seems a bit laggy.The compressed NEWS:
clearGlGroupremoves a group from leaflet and the Leaflet.Glify instances.removeGl**functions were rewritten to correctly remove an element identified bylayerIdclearGlLayersnow correctly removes all Leaflet.Glify instancespopupOptionsandlabelOptions. fix add popup options #83stroke(default=TRUE) inaddGlPolygonsandaddGlPolygonsSrcfor drawing borders. fix Can we draw the polygon borders? #3In the Shiny-App below you will see that the border is not always correct and polygons with holes are problematic. This PR in the upstream repo should fix this.
leaflet.leafglaccepts a single string, a vector of strings or a formula. fix Label argument not working #78...arguments are now passed to all methods in the underlying library. This allows us to setadditional arguments like
fragmentShaderSource,sensitivityorsensitivityHover. fix Clickable space of polyline pop-ups are extremely large #81@inheritParams leaflet::**for identical function argumentsleafgl_json_parserto change JSON-parser via options. Possible values are:jsonify(default)yyjsonrjsonlite::toJSONfor example.fix #103
Shiny-App to Test