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

YAML style options #113

Merged
merged 20 commits into from
Oct 29, 2024
Merged

YAML style options #113

merged 20 commits into from
Oct 29, 2024

Conversation

jimjam-slam
Copy link
Collaborator

Proof of concept here in 8128fb9: if the Lua finds a property defined in the YAML under format.closeread-html.cr-section.style, it writes an HTML style block that creates a CSS variable under this name (prefixed with --cr-), targeting the whole document.

In our SASS, there is a default definition for this var covering the whole doc too, and the corresponding properties in the SASS point to it.

This way:

  1. if a user chooses to manually override the original CSS property (with enough specificity), a default or user-supplied CSS variable value is ignored
  2. if a user overrides the CSS variable value for the whole doc in their own CSS stylesheet, that ought to take precedence over ours (I think)
  3. if a user overrides the CSS variable value for a section, that should take precedence in that section
  4. if a user supplies a YAML option, that should override everything (unless they've overridden the original property, see 1).

I think this makes sense as an appropriate hierarchy of customisation: we can supply a sensible default, provide easy customisation paths for those who want them, and stay out of power users' way.

This needs to be expanded t more properties, and we might want to choose defaults that lean on Bootstrap if it's available (although I'm not sure we can do that from our SASS, at least until Quarto 1.7).

@jimjam-slam
Copy link
Collaborator Author

Have added some more CSS variables:

:root {
  --cr-trigger-background-color: var(--bs-primary);
  --cr-trigger-text-color: var(--bs-light);
  --cr-trigger-border-radius: 1em;
  --cr-trigger-max-width: 527.98px;
  --cr-trigger-outer-margin: 5%;
  --cr-sidebar-background-color: var(--bs-primary);
  --cr-section-background-color: var(--bs-secondary-bg);
  --cr-font-family: var(--bs-body-font-family);
  --cr-font-size: 1.15rem;
}

These are all hooked up in the YAML (no --cr- prefix used there), but not all are hooked up to the corresponding CSS properties (mostly because I wanted to wait until the PR that tinkers with layout and padding is merged).

I've tied some, but not all, of these to Bootstrap CSS variables. Note that the if the corresponding Bootstrap variable is not available, the var() call falls through to the user agent default rather than reverting to some previously defined value. For us that shouldn't be a problem because:

(a) those variables will only be unavailable if theme: none is set
(b) Quarto provides very little CSS if theme: none is set (ie. the main font defers to the user agent stylesheet) anyway
(c) If a user provides values for our CSS variables and scopes it to .cr-section instead of :root, that should take precedence regardless of whether they do it in a stylesheet, in an inline code block or directly on the element.
(d) as long as the no-theme fallback doesn't actively break functionality, I'm fine with it being ugly — theme: none is supposed to be minimal style-wise

Happy to tweak these defaults or names, though!

@jimjam-slam
Copy link
Collaborator Author

jimjam-slam commented Oct 26, 2024

I've caught this up to dev to make the merging easier. I've hooked up the rest of the proposed options, and I've altered the existing gallery docs and home page to use those options. (In the case of NYTimes and Minard, I've trimmed down the CSS where I can, but I wasn't sure how much oft the Minard CSS is actively being used, so I didn't go too wild).

A few things that cropped up while I was working:

  1. Do we actually apply .cr-poem to poems? It looks like we don't anymore — the highlight code appears to function when it finds .line-block inside .sticky, so I've made the poem font function the same way.
  2. I'm having a lot of trouble passing through comma-separated lists of fonts through — when they go through the Lua, everything after the first comma is lost. Not sure why this is, but I'm going to do some Lua debugging to try and pinpoint it.
  3. Occasionally it can be nice to point a CR CSS variable to an existing Bootstrap variable that isn't already being used by default (eg. --bs-light). Unfortunately, it seems like doubling the hyphen functions like an escape for a single hyphen, but three turns into a different kind of dash. The only solution I've found so far is var(\\-\\-bs-light) (with quotes).

@jimjam-slam
Copy link
Collaborator Author

Fixed the issue with strings that contains commas (primarily font lists)! This is ready for review I think, @andrewpbray!

@jimjam-slam jimjam-slam marked this pull request as ready for review October 27, 2024 10:04
@andrewpbray
Copy link
Contributor

Taking a look now!

Copy link
Contributor

@andrewpbray andrewpbray left a comment

Choose a reason for hiding this comment

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

This is awesome!

Two main discussion points:

  1. After seeing how you disarmed the bootswatch theme in the Demos, I'm inclined to either switch from $primary to $dark or just remove those default links to the bs variables.
  2. It might make sense to rename at least some of the trigger- options narrative options to reflect what they're doing in the css.
  3. I'm kinda inclined to remove cr-section under closeread-html in the yaml entirely. The original plan was that under closeread-html there would be cr-section, cr-trigger, and cr-sticky, where you could pass global options to those three core components. As this is maturing though, that doesn't seem like the most useful factoring. Seems like we could just use cr-style to differentiate that this isn't a generic html key.

_extensions/closeread/closeread.lua Show resolved Hide resolved
_extensions/closeread/closeread.lua Show resolved Hide resolved
_extensions/closeread/closeread.scss Show resolved Hide resolved
_extensions/closeread/closeread.scss Outdated Show resolved Hide resolved
--cr-trigger-font-size: 1.15rem;
--cr-poem-font-family: Iowan Old Style, Apple Garamond, Baskerville, Times New Roman, Droid Serif, Times, Source Serif Pro, serif, Apple Color Emoji, Segoe UI Emoji, Segoe UI Symbol;
--cr-sidebar-background-color: var(--bs-primary);
Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at the ends to the demos and noticing that you were opting to override the new default aesthetics that would be pulling from cosmo. I agree with your choice - if you comment those lines out it looks a little too bold - but it makes me wonder if we want to either change the way we link to these bs defaults or remove them (documenting how the user could add them back) (they'd be able to pass cr-sidebar-background-color: "var(--bs-primary)" right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here are some screenshots of how it currently looks on a few bs themes, Minty, Cosmo, and Yeti:

minty
cosmo
yeti

Minty looks ok I think but there isn't enough contrast on Cosmo or Yeti between sidebar and sidebar text. One option would be to swap out cr-sidebar-background-color: var(--bs-primary) for cr-sidebar-background-color: var(--bs-dark). This would create more of an NYT effect, though we'd def want to link this so a light text color.

bw-yeti

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have removed the BS var refs and gone for static default colours! We could adjust them to taste more, but I'm inclined to settle on something and leave further tweaks for the contest, particularly as they're easy to tweak. Left the narrative font family to match Bootstrap.

@@ -5,6 +5,12 @@ description: "Highlight lines or spans of code or line blocks."
format:
closeread-html:
code-tools: true
cr-section:
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed above, this feels like a good amount of work just to get out of a theme that can be oppressive. Seems to me the easiest fix would be to not link to those bs variable by default but allow the user to pass them.

.cr-section .narrative-col {
background-color: rgb(17, 17, 17);
color: white;
min-width: 400px;
Copy link
Contributor

Choose a reason for hiding this comment

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

This, I think, is the line that keeps the width of the sidebar constant while expanding the width of the screen.

_extensions/closeread/closeread.scss Outdated Show resolved Hide resolved
_extensions/closeread/closeread.scss Outdated Show resolved Hide resolved
docs/gallery/examples/auden-poem/index.qmd Outdated Show resolved Hide resolved
@jimjam-slam
Copy link
Collaborator Author

Awesome! Today's a bit hectic, so it might take me some time to respond to things individually, but the tl;dr is I'm happy with all your comments and proposed changes :) Will see if I can get some time tonight or early tomorrow morning to make your suggested changes and document them over on #109 :)

@andrewpbray
Copy link
Contributor

These last two commits are a first take at a sidebar-width option. It occurred to me that there might be two options here: fixing the width of the sidebar, as I've done, or chance the col allocation of in the grid to allow that width to change with screen width.

Should we have both? If so, strong opinions on names?

@jimjam-slam jimjam-slam merged commit 538e5bb into dev Oct 29, 2024
1 check passed
@jimjam-slam jimjam-slam deleted the jimjam-slam/issue99 branch October 29, 2024 22:57
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