qmd-lab / closeread

https://closeread.dev
MIT License
128 stars 5 forks source link

Add spacer shortcode #103

Closed jimjam-slam closed 3 weeks ago

jimjam-slam commented 1 month ago

Fixes #88. Shortcode defaults to height: 40svh:

{{< spacer >}}

... but can be adjusted with a single argument:

{{< spacer 80svh >}}
andrewpbray commented 1 month ago

Very exciting!

Huh, strange. Here's what index.qmd is looking like for me:

Screenshot 2024-10-19 at 6 00 08 PM

And then the warning: WARNING (/Applications/quarto/share/filters/main.lua:20582) Shortcode 'spacer' not found. It looks like all the appropriate files are in docs/_extensions so, let me try to troubleshoot this...

andrewpbray commented 1 month ago

Looks like the issue is that in _extension.yml, shortscodes needs to be nested under format.html in order for it to be bound when a doc calls format: closeread-html. Fixed in 0f43653.

When writing the Minard doc, I recall having a clear idea in my head about the use case that we're designing for. It's.. less clear now. You've implemented it using height on the div while the my kludge used padding-block.

Screenshot 2024-10-19 at 6 24 20 PM

You can see the difference by comparing https://preview.closeread.dev/gallery/examples/minards-map/ to https://closeread.dev/gallery/examples/minards-map/index.html. Do you have a sense of which makes more sense here?

jimjam-slam commented 1 month ago

When writing the Minard doc, I recall having a clear idea in my head about the use case that we're designing for. It's.. less clear now. You've implemented it using height on the div while the my kludge used padding-block. Screenshot 2024-10-19 at 6 24 20 PM

You can see the difference by comparing https://preview.closeread.dev/gallery/examples/minards-map/ to https://closeread.dev/gallery/examples/minards-map/index.html. Do you have a sense of which makes more sense here?

There are some interesting differences here! In your example where you've used a Pandoc div, the padding-block appears to have been hoisted up to the outer enclosing .trigger, and there's nothing inside the inner enclosing .narrative. Is that something our Lua function does?

A consequence of this is that the padding overrides the padding we usually put around trigger content, so you get a bit more control over the ultimate height. The spacer shortcode I've written has our usual padding on top of the height of the spacer, so if you need a smaller spacer, it might not be ideal.

But we need to understand why our manual, empty Pandoc div's style gets hoisted and not the spacer div. You might know this better than me, @andrewpbray!

jimjam-slam commented 1 month ago

When writing the Minard doc, I recall having a clear idea in my head about the use case that we're designing for. It's.. less clear now.

To my mind, the two possible use cases here are to either have the initial text come in a bit later than the first image, or to have a bit of space at the end in the event that you don't finish with content that's been zoomed out (since I don't think we clip content to the .cr-section, so it's easy to have it dangling off the end).

For the former, we might also wish to add a cross-reference argument if we want the first image to come in. Or maybe that's something we should set on the .cr-section itself...

jimjam-slam commented 1 month ago

Also worth noting in the Minard example that you've shifted the padding on triggers to all come after the trigger block, rather than be split between before and after the block! (Nothing wrong with that, just noting it so we can disambiguate that effect from the padding/height of the spacer)

andrewpbray commented 1 month ago

That's a good point about the padding following the trigger block! I remember as I was writing that one: it that felt more natural to be be able to start reading the text before the pan. I also recall thinking that the better way to fix this is to allow another yaml option (or cr-section attribute) used to set the offset value inside scrollama().setup(). As an aside to the main issue here: do you think that'd be a helpful feature? If so we can spin that off to another issue.

andrewpbray commented 1 month ago

I guess I think of a spacer block as being a block that makes no distinction between padding and content; its just an empty spacer block. I guess that has me down on the side of using padding instead of height. That would at least allow the user to think, "I want to budget this all down by about 25% of the screen" and they could directly use that 25% value without needing to know how much padding gets added.

I guess this could be done either though padding or by height and then adding a css rule to .spacer that would set padding to zero. I think I'm slightly in favor of padding because it feels closer to what this spacer is: its just extra padding that's not associated with the same trigger behavior of adjoining blocks.

The funny reverse inheritance thing of the class is done here:

https://github.com/qmd-lab/closeread/blob/2b391101ed6c2c2896874b0bdb8abdd42a67b50b/_extensions/closeread/closeread.lua#L163

I wish I could tell you why I thought that was a good way to do it but ... 😄 🙃

jimjam-slam commented 3 weeks ago

@andrewpbray Have refactored this to use padding! It splits the padding between top and bottom and removes the padding from the parent .trigger (so the height you enter is the one that gets used). Also has some basic validation to ensure you're entering a number potentially followed by some units. Can merge it if you're happy with it!

andrewpbray commented 3 weeks ago

Looks great!