Skip to content

Conversation

gadenbuie
Copy link
Collaborator

@gadenbuie gadenbuie commented Oct 18, 2024

Updates Quarto's Sass rules to set --quarto-border-radius following Bootstrap's $enable-rounded option flag, i.e. not setting Quarto's CSS var if $enable-rounded is not true.

I was wanting hard edges for code blocks and noticed that div.sourceCode uses $border-radius instead of --quarto-border-radius, so I updated that rule as well.

@gadenbuie gadenbuie requested a review from cderv October 18, 2024 15:05
@cscheid
Copy link
Collaborator

cscheid commented Oct 18, 2024

Do you think we can solve this by defining variables with !default instead? That's a little more amenable to static analysis and I hope to lean more into that for helping future users debug their SCSS.

@gadenbuie
Copy link
Collaborator Author

Do you think we can solve this by defining variables with !default instead?

Solve which part with variables? Quarto sets $border-radius here

but the problem is that $enable-rounded is a Bootstrap Sass variable. We can pull that into Quarto's variables if you want.

Or maybe you mean that I replaced $border-radius with the CSS variable here?

border-radius: var(--quarto-border-radius);

If that file is only used in a Bootstrap context (seems like it is), we could also use $enable-rounded there to gate setting border-radius.

@cderv
Copy link
Collaborator

cderv commented Oct 18, 2024

If that file is only used in a Bootstrap context (seems like it is)

Which file were you talking about ?

FWIW _quarto-rules.scss is used in non bootstrap context too

export const quartoRules = () =>
Deno.readTextFileSync(formatResourcePath(
"html",
"_quarto-rules.scss",
));

Used in non bootstrap layer

const rules: string[] = [quartoRules()];

and on bootstrap layers

const rules = [
quartoRules(),

Se we just need to be sure we don't add anything bootstrap related.

@gadenbuie
Copy link
Collaborator Author

If that file is only used in a Bootstrap context (seems like it is)

Which file were you talking about ?

I was talking about src/resources/formats/html/bootstrap/_bootstrap-rules.scss, which from its name appears to be Bootstrap-only. But also it passed the smoke tests without throwing, which is a good sign, whereas before my use of $enable-rounded in _quarto-rules.scss caused smoke test failures.

to keep quarto vars together in devtools
@cscheid
Copy link
Collaborator

cscheid commented Oct 18, 2024

@gadenbuie Sorry, I should have been clearer in my original message.

I'd prefer to solve these kinds of conditional behavior without having to resort to @if statements and variable-exists checks. I say this because the tooling we are building to help people debug SCSS tracks dependencies of variable assignments, and that tracking is a lot simpler when the variables always exist and have a default value.

So, instead of those @if statements, would it be possible to have variable declarations that look like this, more or less?

$enable-rounded: true !default;
$border-radius: if($enable-rounded == true, <whatever-the-default-value>, 0) !default;

The idea is that if someone overrides $enable-rounded or $border-radius by setting new values, we can track and display it in our debugging tools.

@gadenbuie
Copy link
Collaborator Author

gadenbuie commented Oct 18, 2024

We could add $enable-rounded but you wouldn't want to overwrite $border-radius. Maybe you'd add a new $border-rounded variable that looks like your definition of $border-radius conditioned on $enable-rounded.

The reason that you wouldn't want to set $border-radius to 0 is because $border-radius is intended to be part of a set of border radius sizes. The way Bootstrap uses $enable-rounded is to gate border radius settings (e.g. with the border-radius mixin) but users still expect to have $border-radius as part of a set of border radius steps.

@cscheid
Copy link
Collaborator

cscheid commented Oct 18, 2024

Ok, thanks for the additional context and expertise. I retract my concerns.

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

I was talking about src/resources/formats/html/bootstrap/_bootstrap-rules.scss, which from its name appears to be Bootstrap-only.

@gadenbuie yes bootstrap only for this one. 👌

@cscheid cscheid merged commit c1686c8 into main Oct 22, 2024
47 checks passed
@cscheid cscheid deleted the feat/sass-follow-bootstrap-enable-radius branch October 22, 2024 16:29
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