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.

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(brand): Support additional bootstrap layers in defaults #11378

Merged
merged 3 commits into from
Nov 15, 2024

Conversation

gadenbuie
Copy link
Collaborator

@gadenbuie gadenbuie commented Nov 12, 2024

For posit-dev/brand-yml#32

Currently brand.defaults.bootstrap accepts mapping of Sass variables to their values (the defaults layer). In Shiny, however, we found that it was useful to be able to include other layers, like functions, mixins and rules.

This PR updates the Bootstrap bundle created from brand to include these other pieces. defaults is still a nested mapping of Sass variables to default values and the other fields expect scalar strings (this is intended to be a broad initial choice that could be revisited in the future if we need different behavior).

Before

color:
  palette:
    pink: "#ff3d7f"

defaults:
  bootstrap:
    enable-rounded: false
    navbar-bg: black

After

color:
  palette:
    pink: "#ff3d7f"

defaults:
  bootstrap:
    defaults:
      enable-rounded: false
      navbar-bg: black
    rules: |
      h2 { color: $brand-pink; }

@cderv
Copy link
Collaborator

cderv commented Nov 12, 2024

Is this equivalent to providing this using a custom.scss Quarto theme file ?

It feels this is a way to express all layers of the single SCSS file logic quarto introduced as a YAML syntax.

I am just curious to see if I understand correctly - or just missing something. maybe the layering order won't be the same. Also maybe Shiny (bslib) is not supporting single SCSS theme file but is support this new brand.yml ?

@gadenbuie
Copy link
Collaborator Author

Is this equivalent to providing this using a custom.scss Quarto theme file ?

Yeah, you could think of this as if it creates a _brand-bootstrap.scss file.

@cscheid
Copy link
Collaborator

cscheid commented Nov 13, 2024

Thanks, I like this.

One question: should we also support a string for defaults that we simply pass on unchanged, like we do for the other keys in the bootstrap object?

@gadenbuie
Copy link
Collaborator Author

One question: should we also support a string for defaults that we simply pass on unchanged, like we do for the other keys in the bootstrap object?

Yeah I asked myself the same thing when I was working on this as well. I was on the fence, but I decided against the string alternative for two reasons:

  1. The defaults as a nested mapping that becomes Sass variables is different enough that I think it's worth having the tooling support you in that use case.
  2. I'm pretty sure that if you're writing Sass that contains more than just variable declarations in the defaults layer, you're equally able to do the same thing using functions or mixins (where functions comes before defaults or mixins comes just after).

This isn't a super strong position; I'd say these reasons nudged me just over the line into choosing not to include a bare string for defaults. What do you think?

@cscheid
Copy link
Collaborator

cscheid commented Nov 13, 2024

Both of your arguments are true, but I don't think they are related to the reason I'd like to support a string there, namely that I expect someone to try to do it and they'll be surprised when it doesn't.

I wouldn't replace the mapping support: I would allow both and resolve them accordingly.

@gadenbuie
Copy link
Collaborator Author

I wouldn't replace the mapping support: I would allow both and resolve them accordingly.

Sounds good, I added support for a string or an object in b65f9ac.

@cscheid
Copy link
Collaborator

cscheid commented Nov 14, 2024

This needed some conflict fixing after #11440. I'll merge when tests pass.

@cscheid cscheid merged commit 9286ded into main Nov 15, 2024
47 checks passed
@cscheid cscheid deleted the brand/bootstrap-parts branch November 15, 2024 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants