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

link are no more underlined by default #11509

Closed
cderv opened this issue Nov 22, 2024 · 6 comments · Fixed by #11510
Closed

link are no more underlined by default #11509

cderv opened this issue Nov 22, 2024 · 6 comments · Fixed by #11510
Labels
bug Something isn't working html Issues with HTML and related web technology (html/css/scss) regression Functionality that used to work but now is broken. themes Related to HTML theming or any other style related issue (like highlight-style)
Milestone

Comments

@cderv
Copy link
Collaborator

cderv commented Nov 22, 2024

I think this is a regression Quarto 1.6 compared to Quarto 1.5 due to this

to support brand feature regarding typography on link

The default has been changed to inherit


which overwrite previous rule set by bootstrap

Originally posted by @cderv in #11496 (comment)

---
title: Test
format: html
---

Link to [quarto.org](https://quarto.org)

Quarto 1.6

Image

Quarto 1.5

Image

@cderv
Copy link
Collaborator Author

cderv commented Nov 22, 2024

@cscheid is that intended change on our HTML default for brand to match other format ?

@cderv cderv changed the title link are no more underline by default link are no more underlined by default Nov 22, 2024
@cderv cderv added regression Functionality that used to work but now is broken. themes Related to HTML theming or any other style related issue (like highlight-style) labels Nov 22, 2024
@cderv cderv added this to the v1.6 milestone Nov 22, 2024
@gadenbuie
Copy link
Collaborator

I think it's very likely that instead of

$link-decoration: inherit !default;

you'd want to use

$link-decoration: null !default;

That's especially true if we were previously not setting a { text-decoration: $link-decoration; }.

@mcanouil mcanouil added the html Issues with HTML and related web technology (html/css/scss) label Nov 22, 2024
@cderv
Copy link
Collaborator Author

cderv commented Nov 22, 2024

This rule is set by bootstrap in _reboot.scss so it was set previously

And it was used with own bootstrap defined default value

But now the rule is set by us too
https://github.com/quarto-dev/quarto-cli/blob/88819d5b9f092381d5d20a146ac3121dc2435acc/src/resources/formats/html/bootstrap/_bootstrap-rules.scss#L2173-L2179

and the variable apply before bootstrap one

$link-decoration: null !default;

What does null do ? Will it still get the bootstrap default one ? 🤔

@gadenbuie
Copy link
Collaborator

IIUC the goal of defining $link-decoration in _bootstrap-variables.scss is to make sure it is defined before it's used, in particular because _bootstrap-variables.scss are used in revealjs without the rest of the Bootstrap stylesheets.

If I'm right about that, then $link-decoration: null !default is the right move here. It defines the variable without interfering with default order. I.e. it's at least defined but a !default before or after will take precedence.

// Bootstrap: _variables.scss
$def: bootstrap !default;

// Quarto _bootstrap-variables.scss
$def: null !default;

a {
  text-decoration: #{$def}
}

It's something like the above snippet, if Bootstrap's _variables.scss are included, the rule has Bootstrap's default. If not (comment out the bootstrap line), then text-decoration isn't set and there aren't Sass compilation errors. If you didn't define $def at all, then the sheet fails to compile. (Try it here

@cscheid
Copy link
Collaborator

cscheid commented Nov 22, 2024

This is not an intentional change! (Ah, to have good visual testing 🫠)

It's just a matter of replacing inherit with null in that line, right? Let's just do it.

@cscheid cscheid added the bug Something isn't working label Nov 22, 2024
cscheid added a commit that referenced this issue Nov 22, 2024
@cderv
Copy link
Collaborator Author

cderv commented Nov 22, 2024

in particular because _bootstrap-variables.scss are used in revealjs without the rest of the Bootstrap stylesheets.

@gadenbuie _bootstrap-variables.scss is not used in reveals format. No bootstrap related file should be used. Only quarto one could be common, and otherwise, this is some reveal only them file.

This is the case here where it has been added in

$link-decoration: inherit !default;

And in revealjs, links are not underlined by default so this is not a visible change; Revealjs sets it to none in its common theme

.reveal a {
color: var(--r-link-color);
text-decoration: none;
transition: color .15s ease;
}

The new rules will make the element inherit from parent element and so it could be underline or not. 🤷

Also Regarding your example, I believe the layering that happens in quarto is this one

// Quarto _bootstrap-variables.scss
$def: null !default;

// Bootstrap: _variables.scss
$def: bootstrap !default;

a {
  text-decoration: #{$def}
}

it seems to also work.

though it seems to me that as $link-decoration is already bootstrap variable

  • we don't need to define a new rule for brand when in a bootstrap document
  • we need to do it for revealjs, and maybe default to revealjs own default ?

Revealjs theme doesn't use a variable for this but we could also start patching completely the revealjs themes so that it behaves closer to boostrap on all the pieces we need for brand.

cscheid added a commit that referenced this issue Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working html Issues with HTML and related web technology (html/css/scss) regression Functionality that used to work but now is broken. themes Related to HTML theming or any other style related issue (like highlight-style)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants