r-spatial / leafpop

Include Tables, Images and Graphs in Leaflet Popups
Other
113 stars 15 forks source link

Leaflet-popup-container does not respect graph width when the map is embedded in an RMarkdown document #15

Open martinschmelzer opened 4 years ago

martinschmelzer commented 4 years ago

A user on SO reported the following problem:

https://stackoverflow.com/questions/61686111/how-to-fix-distorted-interactive-popup-of-ggplot-figure-in-leaflet-map/61696804#61696804

I tried to locate the problem. One solution might be to set the minimum width equal to the graph width inside popup.js (line 42):

layer.bindPopup(pop, { maxWidth: 2000, minWidth: wdth });

JMLuther commented 4 years ago

Within RStudio viewer and within RMarkdown preview, this is not an issue. this issue occurs during rendering to html output. The CSS fix in the SO response by Martin works well and fixes the issue (thanks for the workaround, btw).

tim-salabim commented 4 years ago

I'll have a look at this over the weekend (hopefully). I also want to move popupTable creation to js, but my understanding of js and css is limited so bare with me. Or feel free to pitch in with a PR if you feel comfortable in that realm. I want to give the user the option to pass the css instructions somehow so that everyone can style their popups as they want. I think all the necessary infrastructure is there, I just struggle to see through the forest...

martinschmelzer commented 4 years ago

I am also not a JS wizard but I'll take a look when I find the time :)

martinschmelzer commented 4 years ago

Locally I added the styles attribute to addPopGraphs which takes raw CSS code like .leaflet-pop-content-wrapper { background-color: blue; }. In addPopupImages, this code is written to a temporary file which then is added as an htmldependency. This works for both stand-alone leaflet widgets and rmarkdown documents containing them. For users who are not very comfortable with CSS, the styles attribute could also be a named list like list(wrapper = "wrapper styles...", content = "content styles...". What do you think about this approach?

tim-salabim commented 4 years ago

If you can wait another week, I should have sorted out most of it. I now know how to add a popupTable on the js side, so just need to figure out how to best set css options from the r side. I am planning on tackling this from Thursday onwards as we have a long weekend in Germany.

One thing to consider here is that we need to be careful as to what happens when you call addpopupgraph more than once with different css settings...

martinschmelzer commented 4 years ago

I am in no rush. Concerning multiple calls to any popup generating method, the className .attribute of layer.bindPopup could come in handy. So leaflet-popup-content-wrapper { ... } would become classForCallXYZ leaflet-popup-content-wrapper { ... }

tim-salabim commented 4 years ago

So leaflet-popup-content-wrapper { ... } would become classForCallXYZ leaflet-popup-content-wrapper { ... }

Nice, I had no idea that CSS works this way. I will try this end of the week and get back to you. If this works, I think we could come up with a very general framework of passing css from R to any htmlwidget... Might even be worth thinking about a stand-alone package for this?

Input a named list -> write to .css -> attach to the widget call

This should be enough, shouldn't it? Are you aware of any exisiting tool for such functionality?

tim-salabim commented 3 years ago

Sorry for the long silence! This should now work as expected in both interactive and markdown mode. We can pass minWidth via the ... notation in addPopupImages (used under the hood in addPopupGraphs). If not supplied, we append the current width of an image in each bindPopup call for each layer (marker, polygon, line etc).

image

image