-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add support for direct importing of plotly figures into DashR apps #71
Conversation
|
...and here's a second example, but using
|
Error in the first example
|
Yep, two extra close parens, just removed them. Thanks! |
element <- layout_objs[[i]] | ||
if (is.list(element) && | ||
"x" %in% names(layout_objs) && | ||
"visdat" %in% names(layout_objs$x)) { |
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.
Why is this in the loop if it's going to replace the whole layout_objs
? Couldn't we do something like
if ("x" %in% names(layout_objs) && "visdat" %in% names(layout_objs$x)) {
obj <- suppressMessages(plotly::plotly_build(layout_objs[[something??]])$x)
return obj[c("data", "layout")]
}
for (i in seq_along(layout_objs)) {
element <- layout_objs[[i]]
# the logic for the last two cases also seems backward as you have it
# in any event you should only need to call is.list(element) once
if (is.list(element) && any(sapply(element, is.list))) {
layout_objs[[i]] <- encode_plotly(element)
}
}
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 logic for the last two cases also seems backward as you have it
# in any event you should only need to call is.list(element) once
If it's a list that contains no other lists, and is also not a plotly
object, we jump to the next value of i
. If it's a nested list, we recurse. It probably makes more sense to rewrite, and omit the preceding else if
:
else if (is.list(element) & any(sapply(element, is.list))) {
layout_objs[[i]] <- encode_plotly(element)
}
I believe we still need two references to is.list
in the line above to check for nested list elements.
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.
After a very helpful discussion with @alexcjohnson, the function is (hopefully) somewhat cleaner and should now handle figure objects which are themselves not nested within a larger list.
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.
fixed in 2ca9b15
R/utils.R
Outdated
obj <- suppressMessages(plotly::plotly_build(layout_objs)$x) | ||
layout_objs <- obj[c("data", "layout")] | ||
return(layout_objs) | ||
} else { |
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.
Is there anything we can bail out on here, and not fall into seq_along
at all? Like perhaps pop the is.list
up above out to wrap everything else:
if(is.list(layout_objs)) {
if("x" %in% names(layout_objs) && "visdat" %in% names(layout_objs$x)) {
...
}
else {
for (i in seq_along(layout_objs)) {
...
}
}
}
layout_objs
I'm thinking particularly about simple values like strings making it here from callbacks.
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.
(Thanks for the update BTW, this already looks much cleaner and I suspect will perform a good deal better as well)
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.
Thanks for the feedback, this feels like a much cleaner, leaner implementation now. 🤞that this is ready to 💃
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.
(hopefully) fixed in f1cc83f
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.
💃 Looks good!
Is there any documentation or a working minimal example about ggplot2 objects in dashR? |
Greetings @heoa; yes, there is a working, minimal example (actually, it's contained within this PR above, but I'll include the complete version here).
|
@rpkyle why did you use https://ggplot2.tidyverse.org/reference/qplot.html
so I guess there should be no big difference to use directly ggplot, instead of qplot, but I woud like to understand this clearly. It would be super useful to see different kind of examples: this was categorial selection. I add some small working examples below I created. Starting time slider
I moved the demos here: #118 |
Either will work equally well; you can use
Both approaches create an object of class
|
This PR proposes to recursively crawl a DashR app's layout, identify components which supply an
x$visdat
sublist, then post-process these using theplotly_build
function in theplotly
R package. The goal is to import and mutate the resulting R object such that Plotly visualizations can be seamlessly included in a DashR app.A brief demo app is included in a comment below.
Closes #62.