Feature/plugin proj4 leaflet #294
Conversation
bhaskarvk
added
type: enhancement
|
Try running This is one approach we can take to preserving backward compatibility; it doesn't involve changing htmlwidgets, but it does make an assumption about how render hooks are stored on the htmlwidget object. |
|
Will do. |
R/leaflet.R
Outdated
| #' @export | ||
| leaflet = function(data = NULL, width = NULL, height = NULL, padding = 0) { | ||
| leaflet = function(data = NULL, width = NULL, height = NULL, | ||
| padding = 0, mapOptions = list()) { |
There was a problem hiding this comment.
I'd like you to rename the mapOptions argument as options (and make its default mapOptions()), and add a mapOptions function that takes crs/minzoom/maxzoom/maxbounds as arguments (center and zoom are covered by setView--or if you prefer we could have center/zoom in both places). This style is what we've used for the other functions and lets us provide autocomplete help.
There was a problem hiding this comment.
Agreed on renaming mapOptions argument to options.
But as for introducing a mapOptions() function, there's already a mapOptions function, which behaves differently than other options() function. tileOptions(), markerOptions() etc. all give back a list . But mapOptions as it's implemented now returns a map. Here's the current implementation
function (map, zoomToLimits = c("always", "first", "never"))
{
if (is.null(map$x$options))
map$x$options <- list()
zoomToLimits <- match.arg(zoomToLimits)
map$x$options$zoomToLimits <- zoomToLimits
map
}If I make it similar to other Options() functions it breaks compatibility.
R/leaflet.R
Outdated
| attr(map$x, "leafletData", exact = TRUE) | ||
| } | ||
|
|
||
| getMapOptions = function(map) { |
There was a problem hiding this comment.
Is there a reason you added this? It's not referenced anywhere else that I can tell.
There was a problem hiding this comment.
If it's OK with you I would rather put it in, it may come handy in certain use cases, none of which are coded in as of now. Besides it makes the code around the map object symmetric.
|
|
||
| # CRS classes supported | ||
| crsClasses <- list( 'L.CRS.EPSG3857', 'L.CRS.EPSG4326', 'L.CRS.EPSG3395', | ||
| 'L.CRS.Simple', 'L.Proj.CRS', 'L.Proj.CRS.TMS' ) |
There was a problem hiding this comment.
Just checking--we'll be able to support (at least) these CRS classes even after we move to Leaflet 1.0?
There was a problem hiding this comment.
Yes, but on a side note this is not an exported list, it is used internally for checking the crsClass argument to mapOptions. So technically it can change w/o breaking API.
But just to be sure, I checked 1.0 code and these projs. are indeed supported there.
|
|
||
| // Merge data options into defaults | ||
| let options = $.extend({ zoomToLimits: "always" }, data.options); | ||
| map = L.map(el, data.options); |
There was a problem hiding this comment.
Before doing this, do you have to check if there's an existing map and destroy it if so?
There was a problem hiding this comment.
will do. I need to read up on htmlwidgets code to truly understand what happens when, and scenarios where there will already be an instance of map.
javascript/src/index.js
Outdated
| } | ||
| }); | ||
|
|
||
| //###HTMLWidgets.widget({ |
There was a problem hiding this comment.
Yup, I left it for the code review part, coz git diff was wanky for this portion, so easier to check new code against commented code in same file. :)
|
Closing this one, preparing a new one as per discussion. |
|
Now part of #298. |
@jcheng5 Not for merging, only for review.
Code sample http://rpubs.com/bhaskarvk/proj4leaflet.
cc @timelyportfolio / @tim-salabim.