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.

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

JohnCoene
Copy link

This is a suggestion to fix #3995.

  1. We use .html() client-side (instead of .text())
  2. R-side, we check if label is NULL if not we convert to character

R/update-input.R Outdated Show resolved Hide resolved
R/update-input.R Outdated Show resolved Hide resolved
@JohnCoene
Copy link
Author

Note: I only include srcts and R file, I did not commit the generated JavaScript, let me know if I should.

R/update-input.R Outdated Show resolved Hide resolved
@cpsievert
Copy link
Collaborator

Thanks @JohnCoene, would you mind looking into the test failures (and fixing them)?

@JohnCoene
Copy link
Author

@cpsievert I believe it's good now.

Comment on lines 359 to 360
const emptyLabel =
Array.isArray(labelContent.html) && labelContent.html.length === 0;
Copy link
Collaborator

@cpsievert cpsievert Mar 15, 2024

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)

Copy link
Author

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

@cpsievert
Copy link
Collaborator

@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.

@JohnCoene
Copy link
Author

Is there anything else I c an do on this PR?

@cpsievert
Copy link
Collaborator

cpsievert commented Apr 8, 2024

Thanks @JohnCoene, please add a NEWS item, then this should be good to merge.

That said, it'd also be great to update our 158-input-labels test to have test coverage of this change (a minimal, still failing, proposal is in rstudio/shinycoreci#262). If you're feeling up to the task, you could also take that PR over the finish line. To run the relevant test locally:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update input - HTML in label
3 participants