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

New editorOptions= argument for lower-level toolbar customization #100

Closed
wants to merge 0 commits into from

Conversation

@JoshOBrien
Copy link
Contributor

@JoshOBrien JoshOBrien commented Jun 5, 2019

The new editorOptions= argument takes a user-supplied list of named options that are ultimately passed on to either leafpm::addPmToolbar() or leaflet.extras::addDrawToolbar(), depending on the value of the editor= argument. (This addresses #92 and incorporates suggestions made in the discussion resulting from #97)

When editor = "leafpm", the list can consist of one or more elements with names "toolbarOptions", "drawOptions", "editOptions", and "cutOptions". For details, see ?leafpm::addPmToolbar.

When editor = "leaflet.extras", allowable names for list elements are "polylineOptions, "polygonOptions", "circleOptions", "rectangleOptions", "makerOptions", "circleMarkerOptions", and "editOptions". For details, see ?leaflet.extras::addDrawToolbar.

Currently, there is no checking or validation of the list passed in to editorOptions=, so users will need to take particular care that the list's structure (including the names of all of its elements) match
with what is expected by the leafpm::addPmToolbar() or leaflet.extras::addDrawToolbar() functions.

Here are few simple examples demonstrating the new argument's usage:

library(sf)
library(mapedit)

##------------------------------------------------------------------------------
## Simple example for testing
##------------------------------------------------------------------------------
x <- list(matrix(c(11,0,11,1,12,1,12,0,11,0), ncol = 2, byrow = TRUE))
pp <- st_sf(geom = st_sfc(st_polygon(x)), crs = 4326)
## ll <- st_sf(geom = st_sfc(st_linestring(matrix(1:4,2))), crs = 4326)


##------------------------------------------------------------------------------
## Passing options to leafpm editor
##------------------------------------------------------------------------------
optsA <- list(drawOptions = list(snappable = FALSE,
                                 hintlineStyle = list(color = "red",
                                                      opacity = 0.5),
                                 templineStyle = list(color = "red")),
              editOptions = list(snappable = FALSE))
x <- editFeatures(pp, editor = "leafpm", editorOptions = optsA)


##------------------------------------------------------------------------------
## Passing options to leaflet.extras editor
##------------------------------------------------------------------------------
## A stripped down toolbar
optsB <- list(editOptions = list(remove = FALSE),
              circleOptions = FALSE,
              markerOptions = FALSE,
              circleMarkerOptions = FALSE,
              rectangleOptions = FALSE)
x <- editFeatures(pp, editor = "leaflet.extras", editorOptions = optsB)


## A more complicated example, resetting several colors
myShapeOpts <-
    leaflet.extras::drawShapeOptions(color = "red", fillColor = "red",
                                     opacity = 0.5, fillOpacity = 0.1,
                                     weight = 4)
optsC <- list(circleOptions = list(shapeOptions = myShapeOpts),
              polygonOptions = list(shapeOptions = myShapeOpts),
              rectangleOptions = list(shapeOptions = myShapeOpts))
x <- editFeatures(pp, editor = "leaflet.extras", editorOptions = optsC)
Copy link
Member

@tim-salabim tim-salabim left a comment

Can we not set allowSelfIntersection = FALSE in the default leafpm options? Also, do we really need to distinguish between polylines/polygon and markers - markers cannot self-intersect anyway?

@tim-salabim
Copy link
Member

@tim-salabim tim-salabim commented Jun 6, 2019

@JoshOBrien this looks good to me. See my comment here. I haven't tested any of it though (just looked over it).

Copy link
Contributor

@timelyportfolio timelyportfolio left a comment

thanks!

##' \code{leafpm::addPmToolbar}.
processOpts <- function(fun, args) {
## Account for special meaning of `FALSE` as arg in leaflet.extras
if(isFALSE(args)) {

This comment has been minimized.

@timelyportfolio

timelyportfolio Jul 17, 2019
Contributor

can we use r-spatial/mapview#177 technique instead? I would prefer not to force users to have to upgrade to newer version of R. I am sensitive to this since this has blocked me inside restrictive employers from using some packages.

This comment has been minimized.

@timelyportfolio

timelyportfolio Jul 17, 2019
Contributor

I added in develop branch 469f620. @tim-salabim, please let me know if you encountered any challenges with this replacement in mapview. Thanks.

This comment has been minimized.

@timelyportfolio

timelyportfolio Jul 17, 2019
Contributor

actually @tim-salabim @JoshOBrien this should be only match FALSE exactly so I changed in bb5e7c9#diff-ad5b576355b0f070a49b21f08b1c27ffR14. sorry.

timelyportfolio added a commit that referenced this pull request Jul 17, 2019
@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 17, 2019

@JoshOBrien @tim-salabim I have merged #98 and this pull into develop branch for a short review period and then plan to merge to master and submit to CRAN. Thanks everyone for the contributions. I love it when open source works as it should.

Sorry for the very long delay. This is not how I anticipate operating going forward.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 17, 2019

@tim-salabim will you please test develop which includes #98 and #100?

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 21, 2019

@tim-salabim I ran CRAN-checks and everything passes except for #102.

@JoshOBrien JoshOBrien closed this Jul 22, 2019
@JoshOBrien JoshOBrien force-pushed the JoshOBrien:master branch from 9f705ee to c3130ca Jul 22, 2019
@tim-salabim
Copy link
Member

@tim-salabim tim-salabim commented Jul 22, 2019

@timelyportfolio. I just arrived in the us. Will try to find some time to test this week.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 22, 2019

@JoshOBrien looks like force push closed this pull request. Was this intentional? If you made changes I will try to add into develop.

@JoshOBrien
Copy link
Contributor Author

@JoshOBrien JoshOBrien commented Jul 22, 2019

@tim-salabim
Copy link
Member

@tim-salabim tim-salabim commented Jul 27, 2019

@timelyportfolio I've had a test run. The only thing I fund missing is that the new editorOptions() argument is not available in drawFeatures. Should be an easy fix though.
Other than that, things seem to behave as expected, though I did not check all available options.
I feel comfortable for this to go live (CRAN) if you are.

Thanks guys for this awesome addition!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.