-
Notifications
You must be signed in to change notification settings - Fork 64
Add input bindings for toolbar buttons and select components #1273
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
base: feat/toolbar-epic
Are you sure you want to change the base?
Conversation
gadenbuie
left a comment
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.
Looking great! I have a handful of comments but they're all mostly pretty small 😄
| # Use a unique ID for the select element to avoid conflicts with standard | ||
| # select binding. The wrapper will have the user-specified id. |
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.
Can you elaborate a bit about what conflicts arise? (Here in this thread first)
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.
Looking back at my notes, it looks like this is because we're wrapping this select element (with the label and icon) and we want to update those as well as the select element within it. We want any update message to go through the toolbar-specific select bindings, not the select binding, which won't know what to do with some of the params we have.
This way, we only register the wrapped select with the toolbar select bindings and avoid having the select double-claimed, if that makes sense?
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.
From discussion: Revert back to the code at bdda4da
should register correctly with data-shiny-no-bind-input added to the select tag, but verify
| disabled = NULL, | ||
| session = get_current_session() | ||
| ) { | ||
| # Validate that label has text for accessibility |
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 you could remove all of the comments in this function
| label_text <- paste(unlist(find_characters(label)), collapse = " ") | ||
| # Verifies the label contains non-empty text | ||
| if (!nzchar(trimws(label_text))) { | ||
| warning( |
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.
Just for consistency
| warning( | |
| rlang::warn( |
| # Process label - wrap it in the same structure as toolbar_input_button() | ||
| # The label content will be updated within the existing .bslib-toolbar-label span | ||
| label_processed <- if (!is.null(label)) { | ||
| processDeps(label, session) | ||
| } else { | ||
| NULL | ||
| } | ||
|
|
||
| # Process icon - wrap it in the same structure as toolbar_input_button() | ||
| # The icon content will be updated within the existing .bslib-toolbar-icon span | ||
| icon_processed <- if (!is.null(icon)) { | ||
| processDeps(validateIcon(icon), session) | ||
| } else { | ||
| NULL | ||
| } |
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.
A small re-ordering; I think this form scans a little better.
| # Process label - wrap it in the same structure as toolbar_input_button() | |
| # The label content will be updated within the existing .bslib-toolbar-label span | |
| label_processed <- if (!is.null(label)) { | |
| processDeps(label, session) | |
| } else { | |
| NULL | |
| } | |
| # Process icon - wrap it in the same structure as toolbar_input_button() | |
| # The icon content will be updated within the existing .bslib-toolbar-icon span | |
| icon_processed <- if (!is.null(icon)) { | |
| processDeps(validateIcon(icon), session) | |
| } else { | |
| NULL | |
| } | |
| icon <- validateIcon(icon) | |
| icon_processed <- if (!is.null(icon)) processDeps(icon, session) | |
| label_processed <- if (!is.null(label)) processDeps(label, session) |
| # Input handler for toolbar_input_button | ||
| toolbar_input_button_input_handler <- function(value, shinysession, name) { | ||
| # Match shinyActionButtonValue class so it behaves | ||
| # like a standard action button for event handlers and input validation |
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.
| # Input handler for toolbar_input_button | |
| toolbar_input_button_input_handler <- function(value, shinysession, name) { | |
| # Match shinyActionButtonValue class so it behaves | |
| # like a standard action button for event handlers and input validation | |
| toolbar_input_button_input_handler <- function(value, shinysession, name) { | |
| # Treat like a standard action button for event handlers and input validation |
| * It extends the standard action button behavior with support for updating | ||
| * the button's label, icon, and disabled state. | ||
| */ | ||
| class BslibToolbarInputButtonBinding extends InputBinding { |
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 wonder if we could import the input button binding directly to avoid having to define some of the basic methods?
| const labelEl = el.querySelector(".bslib-toolbar-label") as HTMLElement; | ||
| if (labelEl && message.label !== undefined) { |
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.
We always include the label and icon containers, right? Just checking that if (labelEl) and similar are about satisfying the type checkers
| } | ||
|
|
||
| getId(el: HTMLElement): string { | ||
| // The wrapper element has the input ID |
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 you can remove all the internal comments in this class as well
| expect_error( | ||
| update_toolbar_input_select( | ||
| "test_id", | ||
| label = "" | ||
| ), | ||
| "`label` must be a non-empty string" | ||
| ) |
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.
This is somewhat of a more recent style adjustment, but I like using expect_snapshot() with error = TRUE for these messages. That way, the error message is captured in the snapshot, and the expectation fails if no error is thrown, even on CRAN.
| expect_error( | |
| update_toolbar_input_select( | |
| "test_id", | |
| label = "" | |
| ), | |
| "`label` must be a non-empty string" | |
| ) | |
| expect_snapshot(error = TRUE, { | |
| update_toolbar_input_select("test_id", label = "") | |
| }) |
On the other hand, this approach introduces the problem that if label = "" made it past the validation, we'd get a different error about missing an active Shiny session. That could be fixed by mocking session, but I'm not sure it's worth it. Up to you!
It would be nice to have the update function on one line in these tests though to make it easier to scan
| expect_error( | |
| update_toolbar_input_select( | |
| "test_id", | |
| label = "" | |
| ), | |
| "`label` must be a non-empty string" | |
| ) | |
| expect_error( | |
| update_toolbar_input_select("test_id", label = ""), | |
| "`label` must be a non-empty string" | |
| ) |
| label_text <- paste(unlist(find_characters(label)), collapse = " ") | ||
| # Verifies the label contains non-empty text |
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.
Given how we now handle label in toolbar_input_select(), I think we might want to bump this up to an error as well. I.e. require that label is provided for accessibility reasons, even if not shown.
Although now I'm remembering that the difference is that in the select input we could also enforce label being a scalar string, where here it could be tags that do meet accessibility requirements. Hmmm 🤔
Fixes #1266
Objectives:
Create input for single select and action button, including update functions.
Decision Log:
update_toolbar_input_select(), there is not a selected choice set by default unless it is also specified using the selected argument. This is consistent with the standardinput_select()s behavior. An alternative would be to automatically select the new choices' first value on update, clearing any selection already made.Additional test and failure cases:
Select Input:
Input Action Button:
Examples and recordings:
Action Button Test App
Screen.Recording.2026-01-05.at.1.01.47.PM.mov
Select Test App
Screen.Recording.2026-01-05.at.1.19.54.PM.mov