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.

Sign up for GitHub

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

fix(ui-containers): Use display: contents #3957

Merged
merged 5 commits into from
Dec 20, 2023
Merged

fix(ui-containers): Use display: contents #3957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac9ded2
fix(ui-containers): Use `display: contents`
gadenbuie Dec 11, 2023
b493c01
`yarn build` (GitHub Actions)
gadenbuie Dec 11, 2023
e93dd1e
feat(conditionalPanel): Gets `.shiny-panel-conditional` class
gadenbuie Dec 11, 2023
be5fc2b
`yarn build` (GitHub Actions)
gadenbuie Dec 11, 2023
1d135b3
docs: Add news item
gadenbuie Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# shiny (development version)

## Breaking changes

* Both `conditionalPanel()` and `uiOutput()` are now styled with `display: contents` by default. This means that the elements they contain are positioned as if they were direct children of the parent container holding the `conditionalPanel()` or `uiOutput()`. This is probably what most users intend when they use these functions, but it may break apps that applied styles directly to the container elements created by these two functions. In that case, you may include CSS rules to set `display: block` for the `.shiny-panel-conditional` or `.shiny-html-output` classes. (#3957)

## New features and improvements

* Added an console that shows some errors in the browser. Also provide better error messages for duplicate input and output bindings. (#3931)
Expand Down
7 changes: 6 additions & 1 deletion R/bootstrap.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,12 @@ wellPanel <- function(...) {
#' }
#' @export
conditionalPanel <- function(condition, ..., ns = NS(NULL)) {
div(`data-display-if`=condition, `data-ns-prefix`=ns(""), ...)
div(
class = "shiny-panel-conditional",
`data-display-if` = condition,
`data-ns-prefix` = ns(""),
...
)
}

#' Create a help text element
Expand Down
2 changes: 1 addition & 1 deletion inst/www/shared/shiny.min.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions inst/www/shared/shiny_scss/shiny.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,3 +469,9 @@ html.autoreload-enabled #shiny-disconnected-overlay.reloading {
/* override anything bootstrap sets for `.nav` */
display: none !important;
}

/* Conditional panels and uiOutput */
.shiny-panel-conditional,
div:where(.shiny-html-output) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you think of any scenarios this might break existing apps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First and most obvious: this would break apps where users were directly styling the conditionalPanel() or uiOutput() container using selectors that target our the emitted markup directly. It's hard to know in advance how those apps would be broken.

In most other cases, I believe setting display: contents is much closer to what users are expecting happens with the children of the conditional panel or UI outputs. In particular that it's much closer to adding the children directly into the layout.

The original issue in posit-dev/py-shiny#881 is a good example that it's not obvious that both approaches add intermediate containers. Outside of flex/grid layouts the impact of the layout container is minimal, but there's greater impact now that we're using both flex and grid more often.

Copy link

@martinamorris martinamorris Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out our shiny application statnetWeb is one of those that breaks with this change.

As the current maintainer, and someone not well versed in this syntax, can you pls tell me if there is a straightforward change to the 2 example code chunks below that will update to the new shiny syntax required:

conditionalPanel(condition = 'input.filetype < 5',
               column(6,
                    br(),
                    fileInput(inputId='rawdatafile', label=NULL, accept='text'),
                    verbatimTextOutput('rawdatafile'))
                )

             conditionalPanel(condition='input.filetype == 3',
                 column(6,
                        uiOutput('pajchooser'))),

Many thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @martinamorris, could you please open a new issue? It'd be helpful if you could describe a bit more how the app has broken and how we could reproduce the issue. It's possible that your issue is related to the change in this PR, but it could also be something else -- this PR didn't involve any syntax changes to conditionalPanel().

Copy link

@martinamorris martinamorris Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, thx. The break occurred with the 1.8.1 CRAN update, and affects the page rendering for pages constructed with conditionalPanel. I followed the link from your 1.8.1 release tag to this commit, but this may not be the culprit.

Will show how I reproduced the issue, with some screenshots.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: #4099

display: contents;
}
Loading