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

page-level themes, specified in the front-matter #443

Closed
wants to merge 1 commit into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jan 5, 2024

All these definitions are valid syntax:

---
theme: dark
---
---
theme: [auto, comic-sans]
---
---
theme:
  - dark
  - comic-sans
---

Any custom theme (as opposed to the default theme indicated in the configuration file) get bundled with rollup as _observablehq/style-{hash}.css (the default theme is unchanged, bundled as _observablehq/style.css).

Note that a page-level theme does not add to the default theme configuration (this is necessary because you don't necessarily want the default theme; we could debate how to indicate that you extend the default, maybe by having a reserved word such as default: [default, solarized] ? This can't be auto since it's not guaranteed that the default theme in the configuration is auto…).

The theme names must be strings containing only letters, numbers, underscores and dashes (\w-). Commas in particular are not allowed because they will be the separator for multiple themes.

On preview, the server needs to know what the hash means, so we pass add a search parameter in the URL, with the theme name(s), possibly joined by a comma, in the clear. The server then does a safety check against the hash, and if it passes builds the combined stylesheet.

On build, this stuff is not necessary, so for the sake of minimizing the HTML, the URL is passed only as style-{hash}.css.

This means, however, that the rendered page is not strictly the same in preview and in build. If we think this is an issue, there are two possibilities, and I have not been able to choose:

  • either leave the search params in build (easy, it will just remove 3 lines of code); a bit ugly, but maybe that's OK?
  • or memoize the information in the preview server when it builds a page. Not sure if that's a good idea, because it would mean that the preview server can't serve a css bundle after a cold-start.

I have not pushed the theme-comic-sans.css file so you can't test this:
theme-comic

(It could be nice to have more built-in theme files to test, but I think it's more important to have a theme resolution mechanism that first checks if the docs/{theme}.css file exists, then the built-in, and then errors. It could be for a different PR though.)

@Fil Fil requested a review from mbostock January 5, 2024 16:02
This was referenced Jan 5, 2024
@mythmon
Copy link
Member

mythmon commented Jan 5, 2024

If I'm understanding this right, are you saying that a project that uses a custom theme would end up with two CSS files: style-{hash}.css (which contains the theme styles) and style.css (which contains the default styles)? If so, could we avoid doing that, and use a different base name for the two files?

@Fil
Copy link
Contributor Author

Fil commented Jan 5, 2024

Page-level themes are defined in each individual page's front-matter, and the (unique) project-level theme is defined in the configuration. You can have both, of course, so we have to generate possibly one different css file for each page that opts in to a custom theme (or group of themes), plus one for the default style. Hence the naming scheme. The hash is 16 chars long and unique to the combination of theme names that a page uses. What would you like to change?

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Functionally this is what I want, but I’m not satisfied with the bundling approach here; I would prefer to make build and preview more similar, and especially to avoid the use of a query parameter in the preview case. Let me take a crack at an alternative.

@mbostock mbostock mentioned this pull request Jan 6, 2024
@mbostock
Copy link
Member

mbostock commented Jan 6, 2024

Counterproposal in #447.

@Fil Fil closed this Jan 6, 2024
@Fil Fil deleted the fil/theme branch January 6, 2024 20:24
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