Support modules in conditionalPanel #1735
Conversation
|
Here is Winston's reproduction of the problem modified to work using the new library(shiny)
condpanelUI <- function(id) {
ns <- NS(id)
tagList(
checkboxInput(ns("checkbox"), "Make module panel visible"),
conditionalPanel(condition = "input.checkbox",
ns = ns,
"This is the module conditional panel")
)
}
ui <- fluidPage(
condpanelUI("foo"),
checkboxInput("checkbox", "Make top-level panel visible"),
conditionalPanel(condition = "input.checkbox", "This is the top-level conditional panel")
)
server <- function(input, output, session) {
}
shinyApp(ui, server) |
srcjs/shinyapp.js
Outdated
| // by nsPrefix. Returns a new object with keys removed and renamed as | ||
| // necessary. | ||
| function narrowScope(scopeComponent, nsPrefix) { | ||
| return mapKeys(pickBy(scopeComponent, function(val, key) { |
There was a problem hiding this comment.
- Feel free to use arrow functions
- I found the indentation a little deceptive somehow, i.e. hard to tell whether the second closure belongs to mapKeys or pickBy
- Can you make it so the lodash-alike functions are actually accessed via a
_object? So we can swap them out in the future and also to make it obvious that the semantics of these functions must not be changed (unless it's to match lodash even more closely).
There was a problem hiding this comment.
The common case by far will be no prefix, exit early in that case?
srcjs/shinyapp.js
Outdated
| var show = condFunc(scope); | ||
| // The data-ns-prefix attribute is always present, but might be empty. If | ||
| // it's empty, no keys are removed or renamed by narrowScope. | ||
| var nsPrefix = el.attr('data-ns-prefix'); |
There was a problem hiding this comment.
Can you check for null/undefined here actually? It's possible for existing HTML templates to have conditional panels hard coded. Our support for such code is not clearly defined but in this case it's so trivial to not break them.
srcjs/utils.js
Outdated
| // pickBy function from lodash. | ||
| function pickBy(obj, predicate) { | ||
| const newObj = {}; | ||
| for (let key in obj) { |
There was a problem hiding this comment.
@wch Can we use Object.keys? I can't remember.
|
Shaping up nicely! It will be a real relief to me when we get this merged! |
|
Introducing Lodash, even with just the functions we use, increases the size of |
|
I agree with @wch fwiw... |
…namespace-conditional-panel
R/bootstrap.R
Outdated
| conditionalPanel <- function(condition, ...) { | ||
| div('data-display-if'=condition, ...) | ||
| conditionalPanel <- function(condition, ..., ns = NS(NULL)) { | ||
| div('data-display-if'=condition, 'data-ns-prefix'=ns(""), ...) |
There was a problem hiding this comment.
Nit: backticks here would be more idiomatic R.
srcjs/shinyapp.js
Outdated
| return Object.keys(scopeComponent) | ||
| .filter(k => k.startsWith(nsPrefix)) | ||
| .map(k => ({[k.substring(nsPrefix.length)]: scopeComponent[k]})) | ||
| .reduce((obj, pair) => Object.assign(obj, pair), {}); |
There was a problem hiding this comment.
Object.assign isn't supported by IE at all, and it's not polyfilled, so we should use $.extend() instead.
…namespace-conditional-panel
|
I think this is now in an IE9-compatible state, but I still need to test and confirm. Will report back soon. |
|
Looks good to me. If it works correctly on IE9, then I think it can be merged. |
|
@wch can confirm: works on IE9. Branch and small demo deployed here: https://beta.rstudioconnect.com/content/2858/ |
…ce-conditional-panel
|
🎉 |
rstudio/shiny-examples@2505726 + large discussion in rstudio/shiny#1735
No description provided.