-
Notifications
You must be signed in to change notification settings - Fork 633
Add modeBarButtonsToKeep argument #1787
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
Conversation
R/layout.R
Outdated
} | ||
|
||
# List of mode bar names from | ||
# https://github.com/plotly/plotly.js/blob/master/src/components/modebar/buttons.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be cool if we could modify the build script I use when updating plotly.js to use this JS source file to programmatically update this R object. I imagine you might be use something like the V8 package to source this file and get the button names as an R object? If that's true then we could store the data as a data object internal to the package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Sure. To get the list of modebar names I simply did some kind of text-mining or scraping like so:
get_modebar_names <- function(url = "https://raw.githubusercontent.com/plotly/plotly.js/master/src/components/modebar/buttons.js") {
modebar_names <- readLines(url)
modebar_names <- stringr::str_trim(modebar_names)
modebar_names <- modebar_names[stringr::str_detect(modebar_names, "^modeBarButtons\\.")]
stringr::str_match(modebar_names, "modeBarButtons\\.(\\w+)")[, 2]
}
modebar_names <- get_modebar_names()
If this is fine for you I can add it to the build script. In that case I would rewrite it using base R functions to get rid of the stringr
dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to go the V8 route if it's possible, but if it's not possible or too much trouble, we can go this route. Also, we should make it part of the the build script (as I linked to above), and write some sensible unit tests to check the value of modebar_names
(i.e., it's a character vector that contains at least the default modebar buttons like zoom2d
, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpsievert is this good to go? i can't find the build script. And if stringr is an unwanted dependency I can rewrite the regex in baseR and paste it here for @trekonom to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build script is at tools/update_plotlyjs.R
. If you can get modebar_names
to be automatically updated when running that script, then I'll consider merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK will do. Thanks
This PR adds a
modeBarButtonsToKeep
argument to the configuration options and closes #1740 .