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.

Sign up for GitHub

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

markerColourIconCheck() fixes error being thrown when allowed colou… #235

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

markerColourIconCheck() fixes error being thrown when allowed colou… #235

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c35b8ed
`markerColourIconCheck()` fixes error being thrown when allowed colou…
merlinoa Apr 15, 2021
de26eba
typo
merlinoa Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions R/google_map_layer_parameters.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ layerId <- function(layer_id){
# to be 'icon'
# @param data the data supplied to the map layer
# @param objArgs the arguments to the function
# @param colour the colour argument for a marker
# @param colour Set to `NULL` to not check the colour. Set to anything other than `NULL`
# to check the colour.
# @param marker_icon the icon argument for a marker
markerColourIconCheck <- function(data, objArgs, colour, marker_icon){

Expand All @@ -134,7 +135,7 @@ markerColourIconCheck <- function(data, objArgs, colour, marker_icon){
}

if(!is.null(colour)){
if(!all((tolower(data[, colour])) %in% c("red","blue","green","lavender"))){
if(!all((tolower(data[["colour"]])) %in% c("red","blue","green","lavender"))){
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should actually be data[[colour]] - without the quotes. Because otherwise it's looking for a column called "colour".

Copy link
Author

Choose a reason for hiding this comment

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

Yea that is what it is doing. The column in the data data frame is called "colour".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you give an example which generates the error you're seeing?

For me this works

library(googleway)

set_key(secret::get_secret("GOOGLE"))

df <- tram_stops
df$colour <- sample(c("red","blue","lavender"), size = nrow(df), replace = T)

google_map() %>%
  add_markers(
    data = df
    , colour = "colour"
  )

Copy link
Author

@merlinoa merlinoa Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The docs for the colour argument state: "string specifying the column containing the 'colour' to use for the markers. One of 'red', 'blue', 'green' or 'lavender'"

If you pass "red", "blue", "green" or "levendar" to the colour argument you get an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The string you pass to the colour argument must specify the column of the data.

Copy link
Author

Choose a reason for hiding this comment

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

Ok -- If that is how the argument should work, IMO it would be more clear to document it something like "the string 'colour' if you are setting the colour of the marker or NULL to use the default marker colour". I made this PR because a client of mine was confused by the docs.

In general though, don't you want to actually pass the colour to the colour argument, not the string "colour"? By passing the string "colour" you can't actually set the colour.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The string 'colour' is just what I named the column of the data. You can name the column of data anything you like.

The colour argument works in the same way as the lon, lat, and many of the others; you use them to tell the the function which column of data holds that variable.

Here's another example where the column my_column holds the marker colours.

library(googleway)

set_key(secret::get_secret("GOOGLE"))

df <- tram_stops
df$my_column <- sample(c("red","blue","lavender"), size = nrow(df), replace = T)

google_map() %>%
  add_markers(
    data = df
    , lon = "stop_lon" ## column name
    , lat = "stop_lat" ## column name
    , colour = "my_column" ## column name
  )

stop("colours must be either red, blue, green or lavender")
}
}
Expand Down