Fix front-end action button label updating logic #4242
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cpsievert
force-pushed
the
fix/update-action-button-label
branch
from
ee5621a to
8b1b374
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
R/input-action.R
Outdated
| icon_separator <- function() { | ||
| tags$span(class = "shiny-icon-separator") | ||
| } |
There was a problem hiding this comment.
I both like this solution and have a hard time with it. It's simple and keeps the icon and text as direct children of the button.
But I don't like introducing the need for both the R and client side to maintain this markup. Would it be problematic to wrap icon and label in elements that are always present, i.e.
<a class="action-button">
<span class="shiny-icon"><!-- icon --></span>
<span class="shiny-label"><!-- label --></span>
</a>There was a problem hiding this comment.
Yea, I'm definitely opting for a less elegant implementation to minimize breaking existing CSS/JS.
It feels somewhat defendable to have a <span class="shiny-icon"> container since it seems less likely that you'd need to select the contents for that, but I'll stew on it a bit more
There was a problem hiding this comment.
Now the logic has been refactored/simplified a bit, I don't hate the implementation as much. It would be simpler if containers were involved, but not so much so that it feels worth it to me (you'd still need both R and JS to be aware of the containers).
There was a problem hiding this comment.
I don't like that the separator has to be included in the initial markup and is also created and removed by the JS. That feels a lot different to me than creating the markup in one place (in R) and reading it in JS. Part of why that feels okay is because we wouldn't need to remove the icon or label containers; we could use CSS to style them appropriately when they're populated or empty, so the markup would be stable throughout the lifecycle.
Anyway, that's definitely not a blocker, I think it's more important to get this fix in. Using the separator doesn't block us from using containers in the future, but using containers does present a risk of breakage if people are using .action-button > * selectors. So I'm entirely okay with merging this as-is for a release and following up with the containers approach if we'd prefer it in the long run.
There was a problem hiding this comment.
Yea, that's fair.
* Follow up to #4242: Restrict icon/label separator spacing to actionButton() * `yarn build` (GitHub Actions) * Add spacing only when both icon and label are truthy * Update snapshot * `yarn build` (GitHub Actions) * Slightly more readable JS * `yarn build` (GitHub Actions) --------- Co-authored-by: cpsievert <[email protected]>
* main: (32 commits) Revert the addition of spacing between `icon` and `label` in `actionButton()` (#4248) Restrict `icon`/`label` separator spacing to `actionButton()` (#4247) Fix front-end action button label updating logic (#4242) Update news Properly handle undefined value for input subscribe callback (#4243) Start new version (#4241) v1.11.0 release candidate (#4232) Follow up to #3996: fix front-end checkbox label updating logic (#4238) feat(InputBinding): subscribe callback now supports event priority (#4211) Follow up to #3996 when label is unspecified (i.e., NULL), don't include it in the message (#4237) Run routine (#4234) chore: #4175 update jquery-ui to 1.14.1 (#4205) Update jQuery to 3.7.1 (#3969) Fix 404 in example 08_html (shiny.min.css) (#4221) Follow up to #3870: fix location of news item (#4233) Bugfix for error found in tests (#3870) Allow update input labels with HTML (#3996) Adds mirai to documentation (#4230) family->given for R Core authorship (#4222) fix(renderPlot): get interactive plotting working with ggplot2 v4.0 (#4228) ...
Closes #4239
For context, this regression was recently introduced by the new HTML label updating feature added in #3996.
In that PR, I overlooked the fact that the
updateLabel()JS helper isn't used to update labels forupdateActionButton().The logic for updating in this case is especially weird since the
icon/labelaren't given their own containers. So, when making the update, we need to remember the previousicon/labelif either of them areNULL.The old logic was basically assuming
iconis an<i>tag. This has annoyed me in the past (for other reasons), so I opted for an approach that doesn't make assumptions about icon markup. While doing so, I also relaxedvalidateIcon()to stop throwing if<i>is anything other than an<i>tag.TODO