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

Fgb highlight fix #101

Merged
merged 4 commits into from
Dec 14, 2024
Merged

Fgb highlight fix #101

merged 4 commits into from
Dec 14, 2024

Conversation

trafficonese
Copy link
Contributor

@trafficonese trafficonese commented Nov 28, 2024

  • This PR fixes issues with highlighting behavior (mouseout) in addFgb by correctly restoring the original style using a deep copy of the style object (with structuredClone and { ...style }). Previously, only the color property was partially supported, and its behavior was incorrect. See FGB attach by group, highlightOptions, label as function #85 for an example app.

  • Introduced the function updateStyleFromProperties to update the highlighted style, if the relevant function argument is NULL and the data has a property with this name (e.g.: color, weight, etc..).

  • Added some explanation to the @details section, outlining how to use direct styling arguments and feature-based styling from data properties. Addresses addFgb color depend on a variable? #87

I also simplified some map$dependencies calls and added Roxygen: list(markdown = TRUE) do the Description. This helps with rendering the docs correctly.

@jpd-defra
Copy link

@trafficonese - thanks very much for adding those details on styling, I was really struggling to work it out!

I can get it to work with color but can't get it to work with fillColor. I have tried adding a column to my fgb named fillColor and fillcolor, yet neither seem to work when fillColor is set to NULL in addFgb.

@trafficonese
Copy link
Contributor Author

trafficonese commented Dec 12, 2024

what about this example - does that work for you? Or is that not what you're looking for?

library(leafem)
library(leaflet)
library(shiny)
library(sf)

url = "https://flatgeobuf.org/test/data/UScounties.fgb"
uscounties <- "./uscounties_polyon.fgb"
fgb <- sf::st_read(dsn = url)
fgb <- st_cast(fgb, "POLYGON")
pal <- colorNumeric("Greens", unique(as.numeric(fgb$STATE_FIPS)))
fgb$fillColor <- pal(as.numeric(fgb$STATE_FIPS))
sf::st_write(obj = fgb, dsn = uscounties, driver = "FlatGeobuf", delete_dsn = TRUE)

ui <- fluidPage(  leafletOutput("map", height = 700))
server <- function(input, output){
  output$map <- renderLeaflet({
    leaflet("map") %>%
      addTiles() %>% setView(-104, 44, 7) %>%
      addFgb(
        file = uscounties
        , fill = TRUE
        , fillColor = NULL
        , fillOpacity = 0.3
        , highlightOptions = highlightOptions(fillColor = "red", fillOpacity=0.8)
      )
  })
}
shinyApp(ui = ui, server = server)

@jpd-defra
Copy link

Thanks for the quick reply. Ah I forgot fill = TRUE. Apologies, that's a silly error on my part.

Would it be possible to add a fillColor example alongside the one you have suggested? Something like:

#'      data$fillColor <- colorNumeric(palette = "viridis", domain = data$var)(data$var)
#'      sf::st_write(obj = data, dsn = "myfile.fgb", driver = "FlatGeobuf")
#'      leafem::addFgb(file = "myfile.fgb", fill = TRUE, fillColor = NULL)
#'      ```

@trafficonese
Copy link
Contributor Author

you mean in the package documentation?

@jpd-defra
Copy link

you mean in the package documentation?

Yeah, alongside the other updates in the @details found in this PR.

@trafficonese
Copy link
Contributor Author

trafficonese commented Dec 12, 2024

@jpd-defra done :) I hope this helps..
There is this line, but it's commented out currently.. so you have to manually set fill = TRUE

leafem/R/file.R

Line 369 in c9c1183

# if (!is.null(fillColor)) fill = TRUE

@jpd-defra
Copy link

@jpd-defra done :) I hope this helps.. There is this line, but it's commented out currently.. so you have to manually set fill = TRUE

leafem/R/file.R

Line 369 in c9c1183

# if (!is.null(fillColor)) fill = TRUE

Thanks very much!

@tim-salabim
Copy link
Member

There is this line, but it's commented out currently.. so you have to manually set fill = TRUE

I vaguely remember that this was commented out because of issues with automatic rendering of lines... But can't remember all the details.

@trafficonese
Copy link
Contributor Author

trafficonese commented Dec 12, 2024

it was about mapview.. #24 (comment)

@tim-salabim
Copy link
Member

Yes, that was it!

As a heads-up, my current plan is to update leafem as much as possible, submit to CRAN and then move on to updating mapview with all the new functionality provided by leafem. Let's see how this will pan out then. I think there should be some way to automatically handle this fillColor and fill issue, but I am too far away from the code base at the moment.

This PR will be merged shortly. I should be able to find some time to get leafem to a submittable state over the next few weeks.

@tim-salabim tim-salabim merged commit 2c3c947 into r-spatial:master Dec 14, 2024
6 checks passed
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