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

Allow more types of sidebar body content #3

Closed
andrewpbray opened this issue Jul 20, 2023 · 24 comments
Closed

Allow more types of sidebar body content #3

andrewpbray opened this issue Jul 20, 2023 · 24 comments
Assignees

Comments

@andrewpbray
Copy link
Contributor

Right now, only code cell content can be dropped into the body. Let's see what it'll take to get working for other block types such images and arbitrary divs.

In code cells, hashpipe options are available to the filter as Div attributes. For images and arbitrary divs, think through whether it'll make sense to flag body content using id prefixes or classes.

@andrewpbray andrewpbray self-assigned this Jul 20, 2023
@jimjam-slam
Copy link
Collaborator

jimjam-slam commented Jul 23, 2023

As part of #4, I forgot to mention that when setting up the sticky content stacking, I've also targeted in SCSS items that either have a data-cr-id attribute (eg. because you used the hashpipe YAML syntax to add cr-id:), or those that have a .cr-id class. You could just add a data-cr-id attribute to a fenced div or image, but I figured it would be clearer to the user if the class matched the YAML attribute.

See what you think—happy to rework this if we land on a different model! I haven't actually tested this with putting an image or other static content in place of the code chunks yet.

@andrewpbray
Copy link
Contributor Author

Ah, right: if we're doing 1A, we don't need to worry about differentiating one sticky / cr-id thing from another, so we could be flagging those elements with a class instead of with an attribute with a unique value. The only awkward thing is that it seems like there's no way tag an executable code cell with a class (unless it's wrapped in a div, which seems a waste).

One approach: we can move all elements with the, say .cr-body (or .cr or .sticky or whatever makes sense) class into a stack in the body column. This could include a fenced div with a code cell in it. We could also allow that to be done as #| .cr-body: true for a more concise syntax.

Thoughts?

@jimjam-slam
Copy link
Collaborator

I tend to like the latter approach, or something like it, where it's an attribute from the user perspective but gets mapped to a class internally. I believe the Quarto team uses that approach for layout (eg. .page-column in a div but #| page: column in a chunk).

If we went with this approach, we could cr- as the "namespace" for the options, so that:

  • Steps are .cr-step or #| cr: step
  • Sticky elements are .cr-sticky or #| cr: sticky

(Not sure why you'd want to make a code cell a step, though; that might be an overgeneralisation 😅)

@andrewpbray
Copy link
Contributor Author

andrewpbray commented Jul 28, 2023

Hey, I like that naming convention! (and am always down for things that generalize needlessly)

One thing that came up when I was fidgeting with the Lua script on the cr-sidebar-add-cr-image branch: certain elements (like an image) can nested within a paragraph, so the has_cr function will need to look a little deeper than just scanning the attributes of the content list of each sidebar div. The question is how deep should we look for the cr and, when we find it, how do we tell how far out back up the tree to crop out and assign to the body block?

My inclination allow any block type in the list of the sidebar div.content (a paragraph, list, image, code cell, generic fenced div, etc) to be dropped in the sticky stack. We can rewrite has_cr to be responsive to the different places where the attribute will show up based on the element type (like finding the image inside the paragraph). If nested block element is assigned the cr attribute, we'll just ignore it.

I can't think of any common use-cases that would break this approach. Once you're in a sidebar div, I can't think of a setting where you need to nest your sticky block element inside another block.

@jimjam-slam
Copy link
Collaborator

Ahh, I see—you mean that I might want to stick, say, an image... but Quarto wraps it in other things (eg. a par) before it gets to the HTML.

For the first part of that, can we use a nested filter to catch things regardless of depth? By which I mean:

  1. Top-level filter runs on each .cr-section block. This gives us our section context. Then, within that, we use walk_block to:
  2. Run an inner filter on blocks, returning them untouched if they're our sticky blocks and deleting them if they're not.

Since the filter returns a deep copy, it could be invertable, and you could run it twice: once to extract the sticky blocks (for moving) and a second time inverted to give you the contents of the body with the sticky content removed.

For the second question (how far do we back out when removing things), I suppose the problem is more, if we remove a nested element, is it a problem if we leave something like an empty <p> behind? Do we know the circumstances in which this happens enough to confidently write some sort of cleanup like "remove empty <p> blocks? Or can we tag them on the way out for possible removal? I guess we need to be careful not to inadvertently remove things that have been tagged by the user (eg. div wrappers for styling purposes) 🤔

All of this is to say that I think I agree with you—I think we can make this work with some minor deviations, if it's just Markdown pars ruining the party!

@andrewpbray
Copy link
Contributor Author

Ahh, I see—you mean that I might want to stick, say, an image... but Quarto wraps it in other things (eg. a par) before it gets to the HTML.

Yep. Take this bit of markdown:

![](pink-desktop.png){cr-id="pink-desktop"}

This gets rendered in the AST to:

Para {
  clone: function: 0x7f80b8f22500
  content: Inlines {
    [1] Image {
      attr: Attr {
        attributes: AttributeList {
          cr-id: "pink-desktop"
        }
        classes: List {}
        clone: function: 0x7f80b8f24b10
        identifier: ""
      }
      caption: Inlines {}
      clone: function: 0x7f80b8f23cb0
      src: "pink-desktop.png"
      title: ""
      walk: function: 0x7f80b8f24110
    }
  }
  show: function: 0x7f80b8f1ef90
  walk: function: 0x7f80b8f227e0
}
  1. Run an inner filter on blocks, returning them untouched if they're our sticky blocks and deleting them if they're not.
    Since the filter returns a deep copy, it could be invertable, and you could run it twice: once to extract the sticky blocks (for moving) and a second time inverted to give you the contents of the body with the sticky content removed.

I'm was reading the Pandoc docs and it looks like the reason Images get wrapped in Para is because the former is considered an Inline element and thus needs to be inside some sort of block. I've checked with Math elements and it looks like the same thing: both $ and $$ are considered Inline elements and thus are wrapped in Para.

I don't know what'll happen if we snip those Inline elements out of their block and put them into a list of blocks, but I'd guess Pandoc will choke. So there's are two things to keep track of: what to do with orphaned Para in the sidebar and how to dress up the sticky content so that it's a block element Pandoc is ok with.

I guess the solution is just to be sure we're detecting that those Para blocks are indeed sticky elements (or rather, contain them as Inlines) and thus should be moved as a block into the body. That should make Pandoc happy and solve the orphaned Para issue.

I'll take a swing at this on cr-sidebar-add-more-block-types.

@andrewpbray
Copy link
Contributor Author

An update:

I was working under a mistaken notion of how walk_block works. Like the primary Lua filter, it will apply a function to all all Pandoc elements that are indicated by the function (Para, Header, Div, etc). To my knowledge there is no catch-all Pandoc type that would cover all block elements, so we'd need to write an exhaustive list of functions for each Pandoc block type.

It feels like there are two general approaches here: using Pandoc filters based on block type, where iteration is done through walk, or conventional iteration in Lua. Seeing as we'd like to conduct our operation on each block regardless of type, perhaps the latter makes more sense.

To be continued...

@andrewpbray
Copy link
Contributor Author

andrewpbray commented Jul 28, 2023

You can take a look at #6 to see:

  1. A new test doc featuring many sticky block types.
  2. A tweak to the filter to catch the image nested within Para

When you render the test doc, you'll see that blue-desktop.png doesn't join the sticky stack (where cr-id is on the Inline image inside the Para block) but pink-desktop.png does (where cr-id at the top-level Div block). I'm guessing the css selector is missing the former? Note that the Math block also worked just fine: the only way that I know how to tag Math with attributes is to wrap it in a div, which then makes everything work fine even though its Div > Para > Math.

One thing this brings up: we could set it up to only allow tags on block elements. Even though you can tag the Image as an Inline (blue-desktop) and its a bit more concise, we could implement and document only the block approach.

Feels like we even if we're only detecting block elements, we'll still need to detect them when they're nested. I'll work on that one next.

@andrewpbray
Copy link
Contributor Author

Ah, I was mistaken. Indeed there is a catch-all function name for all blocks, regardless of size: Blocks.

https://pandoc.org/lua-filters.html#filters-on-element-sequences

So the walk-the-blocks approach should be the way to go here.

@jimjam-slam
Copy link
Collaborator

Mmmm, so if we're walking Blocks, we either need some additional logic to detect inline elements like images or maths, or we need user guidance to wrap them in a div (even if they do take attributes directly, as images do).

@jimjam-slam
Copy link
Collaborator

Ah! One other thing with math is that the following form ("Display Math") is actually a block...

$$
\begin{align}
\hat{y} &= \beta_0 + \beta_1 x + \epsilon \\
&= 3.4 + 1.2 x
\end{align}
$$

... because there is also an explicitly inline form of math, which looks like $x^2$. I'm not sure if there's a way to add attributes to display math without wrapping it in another block, though.

Like you, I couldn't add attributes directly to either form of math. Here's my test on inline:

Finally, here's some inline math:

1. First, on its own: $x^2$
2. Second, using span syntax without the span: $x^2${style="color:red"}
3. Third, wrapped in a span: [$x^2$]{style="color:blue"}
image

@jimjam-slam
Copy link
Collaborator

It looks like this possibility for maths elements has been discussed and rejected in favour of a more general proposal that is pretty far down the priority list (it's been discussed for a decade): jgm/pandoc#5286, jgm/pandoc#684

@jimjam-slam
Copy link
Collaborator

I suppose for our case you could walk both Blocks and Inlines! You would absolutely need to have the user explicitly supply the sticky ID then, though (rather than inferring by position), because there'd be no way to work out which Inlines came before which Blocks, and vice-versa.

@andrewpbray
Copy link
Contributor Author

andrewpbray commented Jul 31, 2023

If I'm reading this right, it looks like when wrapped in $$, you get a Math inline (of mathtype: "DisplayMath") that's made a block because it's wrapped in a Para.

$$
\begin{align}
\hat{y} &= \beta_0 + \beta_1 x + \epsilon \\
&= 3.4 + 1.2 x
\end{align}
$$

Goes to:

content: Blocks {
    [1] Para {
      clone: function: 0x7fea92825010
      content: Inlines {
        [1] Math {
          clone: function: 0x7fea928268b0
          mathtype: "DisplayMath"
          text: "
\begin{align}
\hat{y} &= \beta_0 + \beta_1 x + \epsilon \\
&= 3.4 + 1.2 x
\end{align}
"
          walk: function: 0x7fea92826840
        }
      }
      show: function: 0x7fea92823b20
      walk: function: 0x7fea928252f0
    }

This puts Math in the same category as Images in that they get nested in Para to become a block. The lack of an attribute syntax for Math is a bummer, but wrapping it in a Div should work with this walk-block approach. Very similar to your original outline...

  1. Walk the blocks and save output to body-col
    1.1 If the block has the attribute or if any of its Inline contents have the attribute then
    - return the block else make it nil.
  2. Walk the blocks and save output to sidebar-col
    2.1 If the block has the attribute or if any of its Inline contents have the attribute then
    - make it nil else return the block

For the math setting, the Div > Para > Math outer block will get put into the body-col because of the ID on Div but the inner Para > Math will not. I'm not sure what will happen to the sidebar; if it will include Para > Math or if that branch of the tree will get snipped off when it's upstream block Div > Para > Math gets set to nil. We'll see...

@jimjam-slam
Copy link
Collaborator

Haha, oh no—I didn't actually check that the display math was a block 🤣

@jimjam-slam
Copy link
Collaborator

The other big types of native content we should probably check (to confirm the yare indeed blocks and not inlines) are mermaid.js and other similar diagram blocks!

@andrewpbray
Copy link
Contributor Author

Ah, good idea. I'll add those to the doc I'm testing on.

@andrewpbray
Copy link
Contributor Author

Alright, when you have moment, feel free to play around with.

https://github.com/quarto-lab/close-read/blob/cr-sidebar-add-more-block-types/sidebar-cr-multiple-block-types.qmd

A few things..

  1. I've tinkered with various combos of iteration with pairs, walk_block, and walk, and I finally think I have something that works. walk permits topdown traversal with stopping which seems like just the ticket for getting just the depth of AST that we want in the sticky section without having a duplicate in the sidebar. There are probably a few tweaks that will need to be done once we start looking at more complex examples, but I think this should be a pretty robust way of doing this.
  2. I added mermaid and graphviz blocks to the doc. The former finds its way into the sidebar no prob. The latter isn't working right now. I haven't looked into the file type that's its putting into the HTML file, but my guess is it doesn't like the wrappers that it's in in the body-col.
  3. The blue desktop image isn't getting assigned to the sticky stack. Do you know how to tweak the scss to accomodate it?

@jimjam-slam
Copy link
Collaborator

Sounds good! I'll see if I can have a crack at this tonight after work 😊

@jimjam-slam
Copy link
Collaborator

Sorry, quick update before work on the third issue here! The fundamental problem is that the grid stack layout used by .body_col_stack needs to work with immediate descendents, and the blue image is still wrapped in<p>.

There is a new CSS selector coming down the pipe that lets you target something based on its children (as opposed to targeting the children themselves). But it only has partial browser support.

The modification in #7 works in most modern browsers, but not in Firefox unless you turn a setting on (once Firefox adds it, it'll have about the same support as most modern web features).

I think in the mean time, the safer thing (if possible) is to move .cr-sticky from the img to the containing p, or to unwrap the p entirely!

@andrewpbray
Copy link
Contributor Author

Gotcha - I'm glad you've got a pulse of all the development and implementation of css!

It's not clear to me if we can get away with stripping off the p since Image is considered an Inline (Not sure what pandoc would do with an inline in the list of blocks). Bumping the class to the block element p should be pretty straightforward though. I'll give that a try!

@andrewpbray
Copy link
Contributor Author

There was a bit of a blip here: Para doesn't allow any Attr, so I'm pretty sure you just need to wrap it in a Div. I've done that in #9 and it seems to solve both point 3 (the blue image is now appearing in the sticky stack) and point 2 (graphviz appears in the sticky stack). A quick google suggests that the latter will allow some very fun functionality. =)

I'm inclined to merge #9 then #6 then PR cr-sidebar into main. Any change you'd like to make on this first bit of functionality before then?

What are good next steps? I'm quite certain that you'd be quicker (and better) at figuring out how to get .step classed objects to tigger transitions. There's probably a Lua side to that and a css / javascript side to that. If you have time to take the latter, please have at it. If you won't have time in the coming two weeks, I'm happy to start in on it.

The other thing that I can start working on is how to give users some flexibility with, say, column widths and gaps between paragraphs in the sidebar, both by overriding defaults for the doc in the yaml and overriding doc settings on a block by block basis.

@andrewpbray
Copy link
Contributor Author

andrewpbray commented Aug 2, 2023

(also @jimjam-slam , since I'm realizing its 8 am your time, if you're free for a quick chat right now, let me know!)

@jimjam-slam
Copy link
Collaborator

Yeah, can do! I'll drop you an email in a tic?

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

No branches or pull requests

2 participants