-
Notifications
You must be signed in to change notification settings - Fork 32
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
WIP: Fix labels/border/remove/clear-instances, **Src-functions, etc.. #95
base: master
Are you sure you want to change the base?
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. |
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:
clearGlGroup
removes a group from leaflet and the Leaflet.Glify instances.removeGl**
functions were rewritten to correctly remove an element identified bylayerId
clearGlLayers
now correctly removes all Leaflet.Glify instancespopupOptions
andlabelOptions
. fix add popup options #83stroke
(default=TRUE) inaddGlPolygons
andaddGlPolygonsSrc
for 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
.leafgl
accepts 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
,sensitivity
orsensitivityHover
. fix Clickable space of polyline pop-ups are extremely large #81@inheritParams leaflet::**
for identical function argumentsleafgl_json_parser
to change JSON-parser via options. Possible values are:jsonify
(default)yyjsonr
jsonlite::toJSON
for example.Shiny-App to Test