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

Add spacer shortcode #103

Merged
merged 6 commits into from
Oct 31, 2024
Merged

Add spacer shortcode #103

merged 6 commits into from
Oct 31, 2024

Conversation

jimjam-slam
Copy link
Collaborator

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

{{< spacer >}}

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

{{< spacer 80svh >}}

@andrewpbray
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator Author

jimjam-slam commented Oct 20, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Contributor

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:

block = wrap_block(block, {"narrative"})

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

@jimjam-slam
Copy link
Collaborator Author

@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
Copy link
Contributor

Looks great!

@jimjam-slam jimjam-slam merged commit 12e6098 into dev Oct 31, 2024
1 check passed
@jimjam-slam jimjam-slam deleted the feature-spacers branch October 31, 2024 03:50
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