-
Notifications
You must be signed in to change notification settings - Fork 83
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
refactor: Move ui.Theme.from_brand()
Sass code into an .scss
file
#1792
base: main
Are you sure you want to change the base?
Conversation
shiny/ui/_theme_brand.py
Outdated
self._insert_sass("brand.color.palette:defaults", brand_color_palette_defaults) | ||
self._insert_sass("brand.defaults:defaults", brand_bootstrap_defaults) | ||
self._insert_sass("brand.color:defaults", brand_color_defaults) | ||
self._insert_sass("brand.typography:defaults", brand_typography_defaults) | ||
self._insert_sass("brand.color.palette:rules", brand_color_palette_rules) |
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.
Any chance this could be done with .add_defaults()
and .add_rules()
instead of ._insert_sass()
?
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.
Maybe? But it'd be a quite a bit more complicated and would reduce the portability of the Sass file. Obviously we can't add a new section without writing new Python/R code, but with this implementation we can adjust this Sass file directly without having to then carefully considering the order of operations that need to happen in the Python/R code.
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 also like that you can look at the Sass file and have a sense of the pieces that are going to be added.
Also string substitution isn't elegant but it's certainly easy to do in both Python and R and both implementations will basically be the same thing. It's likely that using the Sass layer mechanisms will work out to look pretty similar in both languages, but it certainly increases the context window of things you need to understand.
In other words, I'm going for a solution that, as much as possible, fits into one Sass file.
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.
Anyway, that's what I was thinking but I rewrote it to use standard Sass methods and I don't hate it. 4ddf0f5
Removes the `_insert_sass()` method to use standard `ui.Theme()` methods
$body-secondary-color: $brand_color_secondary !default; | ||
$body-secondary: $brand_color_secondary !default; | ||
$body-tertiary-color: $brand_color_tertiary !default; | ||
$body-tertiary: $brand_color_tertiary !default; |
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.
Maybe let's save this for another PR, but from a quick look, it seems $body-secondary
and $body-tertiary
don't exist?
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.
There's a good chance I copied this from the Quarto implementation. I'll check.
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.
Yup, came from here: https://github.com/quarto-dev/quarto-cli/blob/bf550cf7049f19d486b7410cb746b70367d0c359/src/core/sass/brand.ts#L31-L34
Happy to leave this for a follow-up PR.
# * brand.color.palette | ||
# * brand.defaults (Brand-defined Bootstrap defaults) |
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 would have expected the ordering here to be swapped (i.e., Bootstrap defaults first since they're the most specific). What's the rationale for the palette coming 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.
The goal is to be able to say something like $navbar-bg: $brand-blue
, which requires this order:
$brand-blue: "#2233ff";
$navbar-bg: $brand-blue;
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.
Ah, ok, that feels worth it. A mention of that capability in the Sass would be great.
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.
idk this feels a lot like the kind of comment that's explained by the code. brand.color.palette
is only the brand's named colors and its Sass variables come before brand.defaults
because otherwise you couldn't use them under brand.defaults
. If it were the other way around, I think it'd need to be commented.
Co-authored-by: Carson Sievert <[email protected]>
shiny/ui/_theme_brand.py
Outdated
# Currently, brand.color fields are directly named after Bootstrap vars. If | ||
# that changes, we'd need to use a map here. These values can't be set to | ||
# `null !default` because they're used by maps in the Bootstrap mixins layer | ||
# and cause errors if a color is `null` rather than non-existent. |
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.
Could it be that this is the problem? https://github.com/rstudio/bslib/blob/112efd0e/inst/lib/bs5/scss/_variables.scss#L303
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.
Yup! Fixed in rstudio/bslib#1149
The main goal is to move as much Sass code and logic out of Python and into a single
_brand-yml.scss
layer file, building on #1790.There's a small amount of code insertion that still needs to happen, these places are marked in the Sass file with special comments, e.g.
As part of this, we now push a set of brand-specific Sass variables and primarily use these to set other values, e.g.
Note that we can't use the
null !default
set up for the Bootstrap core variables like$primary
or$secondary
etc. because thenull
value ends up infiltrating into places where it's not valid.