-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat(shiny-preset): Notifications, progress bars, and modals #754
Conversation
Just a heads up, I think it could make sense to pull in at least some of this into shiny.scss (partly because some of these things feel like good general improvements, and also because we're also going to educate people to upgrade Shiny if they want things to work well with the new dark color mode) |
Just for reference, here's a related shiny issue rstudio/shiny#2988 |
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
R/bs-theme.R
Outdated
shiny = sass_layer( | ||
mixins = sass_file(path_inst("shiny-scss", "bs5", "_variables.scss")) | ||
), |
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.
Is this here (and not in the preset) because we think these styling decisions should generally apply to Bootstrap 5? In that case, it seems that this (as well as the rules below) should live in Shiny (but only applied when $bootstrap-version
is a suitable value).
Doing this should help prevent situations like rstudio/shiny#3905, which still is a potential problem for other Shiny Sass like selectize.js. I was thinking we should provide something like a shiny:::compileDependencies(theme)
that PyShiny
could use to compile all of Shiny's Sass->CSS with whatever theme it wants.
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.
Yeah, in general I realized that these defaults improved all Bootstrap/Bootswatch 5 themes.
I'm not sure I agree that we should move them into Shiny, however. I don't think it would help with the linked issue (we misdiagnosed the problem when we thought it was about missing variables).
My primary concern is that Bootstrap 5 comes from bslib and the more we push Bootstrap-version-specific rules and variables out of bslib, the further they are from the place that drives the change. Each incremental change we push out of bslib becomes one more place we have to remember to evaluate (or accidentally forget) for changes later, e.g. if there's another BS 5 patch release.
That said, maybe there's a wrinkle in how shiny notification styles are included from the Shiny side that I'm missing.
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 now done in rstudio/shiny#3914. Once I removed it from bslib in this PR, however, we now enter a weird state where a newer bslib (i.e. with the changes in this PR) paired with an older version of Shiny won't have the right notification styles -- the colors will use the default shiny styles and not the newest variables.
I guess that's an intrinsic trade-off in moving the Bootstrap 5 defaults over to Shiny, but I wanted to call it out.
Closes #752
Closes #753
Preview
Progress
Kapture.2023-08-17.at.11.26.53.mp4
Notifications
Modal
@gregswinehart suggested we add a close x in the upper right corner, but this goes beyond what we could do with CSS from bslib (it would require changes to the markup and the
shiny::modalDialog()
function signature).Also, I added a small amount of blur to the modal backdrop. I like it and it's in line with other Posit design elements, but I'd also be fine removing it if others hate it.
Notes
I kept the colored text for the info, warning and error notifications, and I made sure that they have a contrast ratio of at least 7+.
Also we can't replace the close button icon, but I think we got reasonably close. I updated the styles to also use an appropriate pointer with a touch area that's larger than the bounding box of the "x".
Kapture.2023-08-17.at.11.30.39.mp4