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 config #442

Merged
merged 16 commits into from
Jan 8, 2024
Merged

theme config #442

merged 16 commits into from
Jan 8, 2024

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Jan 5, 2024

Implements the project-level theme option as a shorthand alternative for the style option. TODO implement the equivalent page-level options, allowing multiple style bundles for the project. Done.

Ref. #440 (comment)

@mbostock mbostock requested a review from Fil January 5, 2024 00:36
@mbostock
Copy link
Member Author

mbostock commented Jan 5, 2024

I’m not sure how we’d identify (name) the different style bundles for the page-level options… 🤔 Maybe the answer is that we load the themes (if any) and the stylesheet (defaulting to default.css) as separate requests? Need to think more.

@Fil Fil mentioned this pull request Jan 5, 2024
@Fil
Copy link
Contributor

Fil commented Jan 5, 2024

I've tried to address the todo in #443; this looks totally viable.

@mbostock mbostock marked this pull request as ready for review January 6, 2024 18:54
path: string,
resolver: ImportResolver
): Promise<Html> {
const stylesheets = new Set<string>(["https://fonts.googleapis.com/css2?family=Source+Serif+Pro:ital,wght@0,400;0,600;0,700;1,400;1,600;1,700&display=swap"]); // prettier-ignore
Copy link
Member Author

@mbostock mbostock Jan 6, 2024

Choose a reason for hiding this comment

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

If we generated the style bundle here, we could scan it for external imports and add preloads here to fix #423.

@mbostock
Copy link
Member Author

mbostock commented Jan 6, 2024

Merged with #447 and ready for review now @Fil. 🙏

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

we need to update the docs!

@Fil
Copy link
Contributor

Fil commented Jan 7, 2024

some docs in #448

@mbostock
Copy link
Member Author

mbostock commented Jan 8, 2024

The path resolution of the style option isn’t quite correct here. For the project-level option, it should either be resolved relative to the config file or the source root; for the page-level option, it should be resolved relative to the page (the Markdown file). The latter is working as intended, but the former is not.

@Fil
Copy link
Contributor

Fil commented Jan 8, 2024

I think (??) there is still an issue where using

observable dev --root=docs --config=config.js

is not loading the same configuration file as
observable dev --config=config.js

even though docs is the default for the --root option?

I'll double-check later.

@mbostock
Copy link
Member Author

mbostock commented Jan 8, 2024

You mean observable preview right? It looks like that currently errors if you try to pass both --root and --config because it parses the arguments incorrectly. But also I don’t think it’s meaningful to set both options, since the config file itself says where the root is, and the --root option is just a shorthand alternative to writing a config file.

@Fil
Copy link
Contributor

Fil commented Jan 8, 2024

Yes. I have these 4 variants, that use the default values either implicitly or explicitly, and so I'd expect them to give the same result:

  • [a] yarn tsx bin/observable.ts preview
  • [b] yarn tsx bin/observable.ts preview --config=observablehq.config.ts
  • [c] yarn tsx bin/observable.ts preview --root=docs
  • [d] yarn tsx bin/observable.ts preview --root=docs --config=observablehq.config.ts

However they don't. Only [a] and [b] use /docs/ and /observablehq.config.ts as expected.
But:

  • [c] uses /docs/ and /docs/observablehq.config.ts
  • [d] errors with observable: Unexpected argument 'preview'. This command does not take positional arguments. See 'observable help preview'.

Is this an artifact of how I'm testing? It's not necessarily a blocker in the sense that it's not blocking any way you'd want to do things, but I find this confusing.

@mbostock
Copy link
Member Author

mbostock commented Jan 8, 2024

[c] and [d] aren’t equivalent to [a] and [b], aside from the splice error (which I fixed). The source root is specified by the config file, not the --root option. The --root option is typically used as a shorthand alternative to --config. So, --root=docs will look for docs/observablehq.config.ts, and so will --root=docs --config=observablehq.config.ts.

@mbostock mbostock enabled auto-merge (squash) January 8, 2024 20:05
@mbostock mbostock merged commit 80f7b34 into main Jan 8, 2024
2 checks passed
@mbostock mbostock deleted the mbostock/theme branch January 8, 2024 20:06
@mbostock mbostock mentioned this pull request Jan 8, 2024
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.

2 participants