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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@timelyportfolio timelyportfolio Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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

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

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

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

@tim-salabim
Copy link
Member

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

@timelyportfolio
Copy link
Contributor

@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 commented Jul 22, 2019 via email

@tim-salabim
Copy link
Member

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants