-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Allow update input labels with HTML #3996
base: main
Are you sure you want to change the base?
Conversation
Note: I only include |
Thanks @JohnCoene, would you mind looking into the test failures (and fixing them)? |
@cpsievert I believe it's good now. |
srcts/src/utils/index.ts
Outdated
const emptyLabel = | ||
Array.isArray(labelContent.html) && labelContent.html.length === 0; |
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 think this check might also now need to check for empty string? I noticed that this PR will break this "clear label" feature:
library(shiny)
ui <- fluidPage(
checkboxInput("id", label = "foo")
)
server <- function(input, output, session) {
updateCheckboxInput(inputId = "id", label = character(0))
}
shinyApp(ui, server)
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.
Ah good catch. Do we still need Array.isArray
check? I'm not sure what's for to be honest; could we/can we pass a vector of length greater than 1 to label
?
Asking because I wonder if it can't be simplified to just labelContent.html.length == 0
since "".length == 0
is true
@JohnCoene I noticed #3996 (comment) when having a look into modifying one of our testing apps to account for this feature. I've got a start on that here rstudio/shinycoreci#262 and it'd be awesome if you wanted to review/fix/improve that test app. |
Is there anything else I c an do on this PR? |
Thanks @JohnCoene, please add a NEWS item, then this should be good to merge. That said, it'd also be great to update our cd shinycoreci/inst/apps/158-input-labels
R -e "shiny::runTests()" Screenshot differences are to be expected -- as long as the screenshot changes look good locally and there are no other failures than we can merge that PR as well |
This is a suggestion to fix #3995.
.html()
client-side (instead of.text()
)NULL
if not we convert to character