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

theme - export two more css variable about code block color #10722

Closed
wants to merge 2 commits into from

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Sep 5, 2024

This PR just export two more CSS variables to pass the information about color used for code blocks.
This complements the other variables already defined for extension.

closes #10717 for quarto side. quarto-live will need to do the rest. cc @georgestagg

This needs to be checked against various example to see if that is enough to handle all themes.
So opening as a draft

This is useful for extension like quarto-live to help style code blocks
@cderv
Copy link
Collaborator Author

cderv commented Sep 5, 2024

It seems like this is not the right place to export those variables 🤔

@cderv
Copy link
Collaborator Author

cderv commented Sep 5, 2024

Now doing this right where the variables are defined, which avoid the problem.

This adds new :root { definition elsewhere in the CSS. Not sure this is the right place to do this.

We do export in several ways. One of theme is even for typescript extra addition also

export const quartoGlobalCssVariableRules = () => {
return `
$font-family-monospace: SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace !default;
/*! quarto-variables-start */
:root {
--quarto-font-monospace: #{inspect($font-family-monospace)};
}
/*! quarto-variables-end */
`;
};

Anyhow, discussed with @cscheid : We should export all SCSS variable as CSS variable, namespaced properly.

So I'll leave this as draft, and we can probably do the export of everything really soon.

@cscheid
Copy link
Collaborator

cscheid commented Sep 5, 2024

Yes. Specifically, well use the cssVarsBlock code from a branch:

https://github.com/quarto-dev/quarto-cli/blob/feature/sass-analyzer/src/core/sass/add-css-vars.ts#L18-L31

@cscheid
Copy link
Collaborator

cscheid commented Sep 20, 2024

This pull request will make all SCSS color variables available for CSS under the --quarto-scss-export- prefix, see https://github.com/quarto-dev/quarto-cli/pull/10727/files#diff-2b4c198af19970ab5cb8ebcc2ad79da82d5f13f3ed648ed3891b46d72ba84aab. I think we can close this, then?

@cscheid cscheid closed this Sep 20, 2024
@cderv cderv deleted the theme/export-code-variable branch September 23, 2024 08:06
@cderv
Copy link
Collaborator Author

cderv commented Sep 23, 2024

@georgestagg I think you can now check for existence and value for

  • --quarto-scss-export-code-block-bg
  • --quarto-scss-export-code-block-color

You can try highlight-style: breezedark to have both.

@cscheid I did find those variable using an example and looking at the .css file.

Initially I would have expected the value to be the one in SCSS, which is $code-block-bg-color, but it seems we use the -color.

$code-block-bg exists indeed, but can be true, false or a color from our doc. It is used to drive different if condition in scss. I understood our --quarto-scss-export was for color variable only for now.

Just sharing as feedback in case this is not expected.

@cscheid
Copy link
Collaborator

cscheid commented Sep 23, 2024

$code-block-bg exists indeed, but can be true, false or a color from our doc. It is used to drive different if condition in scss. I understood our --quarto-scss-export was for color variable only for now.

Your understanding is correct.

My analyzer won't guarantee that a variable with potentially different types will be exposed. Ideally, we'd write our SCSS to have more stable types, and then the analyzer accurately detects what's going on. It's possible that the analyzer is getting confused with the different types happening and it's emitting bad CSS. If that's the case, it's a bug I should fix. If it's emitting correct CSS but with a weird, unexpected CSS variable, then I think it's an annoyance rather than a bug.

@cderv
Copy link
Collaborator Author

cderv commented Sep 23, 2024

If that's the case, it's a bug I should fix. If it's emitting correct CSS but with a weird, unexpected CSS variable, then I think it's an annoyance rather than a bug.

For now I only identified the latter. I'll watch out for the former in cases where $code-block-bg is set to something specific;

Thanks for the clarification !

@georgestagg
Copy link

@cderv Sorry that it's taken me a little while to get back to this, I have some feedback below.


This seems to mostly work, but I'm coming upon an issue when I have highlight-style: a11y-dark set. With this, the CSS variable --quarto-scss-export-code-block-bg is defined, but --quarto-scss-export-code-block-color is not defined.

I think there might be a couple of problems colliding here. Firstly, in the theme file for a11y-dark, the default text colour is set to null, which seems incorrect to me:

{
"text-color": null,
"background-color": "#2b2b2b",
"line-number-color": "#97947a",

However, I think things work out OK when rendered because the Normal text style sets the colour as required during highlighting:

"text-styles": {
"Normal": {
"text-color": "#f8f8f2"
},

These theme text styles are exported as --quarto-hl-*. I use this to pick up the colours for my own highlighter for Quarto Live:

Object.keys(textStyles).forEach((styleName) => {
const abbr = kAbbrevs[styleName];
if (abbr) {
const textValues = textStyles[styleName];
Object.keys(textValues).forEach((textAttr) => {
switch (textAttr) {
case "text-color":
lines.push(
` --quarto-hl-${abbr}-color: ${
textValues[textAttr] ||
"inherit"
};`,

However, notice that for the Normal text style, the abbr value is the empty string, at the bottom of this Record :

export const kAbbrevs: Record<string, string> = {
"Keyword": "kw",
"DataType": "dt",
"DecVal": "dv",
"BaseN": "bn",
"Float": "fl",
"Char": "ch",
"String": "st",
"Comment": "co",
"Other": "ot",
"Alert": "al",
"Function": "fu",
"RegionMarker": "re",
"Error": "er",
"Constant": "cn",
"SpecialChar": "sc",
"VerbatimString": "vs",
"SpecialString": "ss",
"Import": "im",
"Documentation": "do",
"Annotation": "an",
"CommentVar": "cv",
"Variable": "va",
"ControlFlow": "cf",
"Operator": "op",
"BuiltIn": "bu",
"Extension": "ex",
"Preprocessor": "pp",
"Attribute": "at",
"Information": "in",
"Warning": "wa",
"Normal": "",
};

Since the abbr is the empty string for the Normal text style, the if (abbr) { block does not execute, and so the Normal text style is not exported as a CSS variable for me to use.

Is there some way we could add an abbr for the Normal text style, or otherwise export the value under --quarto-hl-normal-color or similar? With that, I can add a fallback in my Quarto Live highlighter for when the default text-color value is null, as in the a11y-dark theme.

Or, alternatively, I think we could set "text-color": "#f8f8f2" on line 2 in the a11y-dark.theme file, but we'd have to check the other theme files to make sure there's no similar issue in the others.

@cderv
Copy link
Collaborator Author

cderv commented Dec 16, 2024

Thanks a lot for this feedback and the analysis !

Or, alternatively, I think we could set "text-color": "#f8f8f2" on line 2 in the a11y-dark.theme file, but we'd have to check the other theme files to make sure there's no similar issue in the others.

I need to understand what this value represent. If I look to other themes, there is either no value (like in atom-one-dark.theme), or a null value like in a11y themes, or arrow themes, or haddock. but some themes have a value (like breezedark or espresso or kate)

I should read the spec for those theme file to understand. 🤔

Since the abbr is the empty string for the Normal text style, the if (abbr) { block does not execute, and so the Normal text style is not exported as a CSS variable for me to use.

kAbbrevs have meaning for the highlighting and are taken from list upstream
https://github.com/jgm/skylighting/blob/2e2220a8a180dc9e1ed8b8e346d7ebe85db8bb76/skylighting-format-blaze-html/src/Skylighting/Format/HTML.hs#L129-L160

or otherwise export the value under --quarto-hl-normal-color or similar?

However, I think we can handle Normal specifically and at least do that. I think this would be ok to add
--quarto-hl-normal-color or maybe just --quarto-hl-color ?

@georgestagg
Copy link

However, I think we can handle Normal specifically and at least do that. I think this would be ok to add
--quarto-hl-normal-color or maybe just --quarto-hl-color ?

Thanks, I think exposing the value in this way would work well for me. I don't have a preference in terms of naming, happy to use whichever is decided.

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.

highlight-style treated differently for format: * and format: live-*
3 participants