qmd-lab / closeread

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

Handling steps and cr attribute syntax #32

Closed andrewpbray closed 4 months ago

andrewpbray commented 5 months ago

@jimjam-slam Here's a proposal for a tweak on our syntax. Here are some examples that cover transitions and some focus effects.

{change-to=“sticky1” transition=“fade-in”} (this could be a default value) {change-to=“sticky1” transition=“fade-left”} {focus-on=“sticky1” highlight-spans=“myspan”} {focus-on=“sticky1” highlight-lines=“1-3”} {focus-on=“sticky1” zoom-to=“100, 120” zoom-level=“2”}

These names are all just first cuts, but there are a few key changes to the functionality:

  1. They always indicate the sticky element that should get .cr-active. This allow a focus effect on an element other than the one that is currently / previously active.

  2. They never indicate what you're transitioning from. This is to make it agnostic to scrolling up or scrolling down.

  3. Focus effects don't differentiate between the block type All effects wouldn't work for all block types, but I'd imagine we could implement highlight-lines to work in evaluated code cells, verbatim code cells, line blocks, display math; highlight-spans to work in line blocks, paragraphs, and verbatim code cells; and zoom-to to work for ... all block types?

One thing that's noteworthy is we'd no longer be using classes. Not sure if that's strange or not? I don't have a clear sense of how map the components of a function (highlight(object, lines)) to classes and attributes.

There are a couple implications of this for the way the code could be structured, but the main one, I think/hope, is that we might be able to simplify the replaying approach to guard against too-quick scrolls. The idea would be to form the set of allSteps and allStickies when DOMContentLoaded but before the scroller is initialized. Inside recalculateActiveSteps, we could add .cr-active to the value of change-to/focus-on and remove it from all other StickyObjects.

Thoughts? Is this an improvement or a step backwards?

jimjam-slam commented 5 months ago

This makes sense to me! When we originally wrote this, I believe we chose to have the from target as well because we thought people might want to layer several elements on top of each other at once. I actually think the simplification (both in terms of syntax and in terms of implementation) would be worth dropping this!

andrewpbray commented 5 months ago

Ah right. I remember that conversation!

If we wanted to, I feel like we could probably extend this syntax to accommodate the layering use case, perhaps by adding something like keep="sticky2,sticky3".

For now I'll start a branch to implement this. Any suggestions for changes to the attribute names? I started with transition-to but it seemed a bit long.

jimjam-slam commented 5 months ago

Tbh I like the attribute names you're proposing! Also if you're okay with a comma-delimited list of IDs, we could still use change-to for the layering case 😊

andrewpbray commented 5 months ago

Ha, you're right 👌

andrewpbray commented 5 months ago

@jimjam-slam As I'm implementing this in JS in rename-steps, I'm realizing that an alternative would be to strike change-to and identify all steps by the presence of focus-on. Then when the step enters the viewer, it will add .cr-active to the corresponding sticky. If additional attributes are attached to that step (like highlight-lines), we run the corresponding function to perform that action.

Thoughts? Is there value in making clear to the user the distinction between a transition and a focus effect?

andrewpbray commented 5 months ago

Thinking this through, I encountered this example, which I thought at first would be bad form that we'd want to throw an error for but now I'm not sure:

:::{focus-on="pink" transition="fade-left" highlight-lines="1-4"}
blah blah
:::

That is: a step that combines attributes for both a transition and a focus effect. But now I'm thinking... that's maybe ok! I can think of use-cases when that's what the user would sensibly want.

Allowing this also leads to simple JS code: we read down the attribute list for a step and run each of the corresponding functions. The overall effect will be the composition of several effects, but I think they should compose in visually acceptable ways.

jimjam-slam commented 5 months ago

Again, apologies for me diving into the PR before I catch up on the discussion! Ofc you've already considered most of the things I've commented on there 🤦🏻‍♂️

I do like being able to combine change-to and focus-on! Separating them might encourage people to try to give separate IDs for change-to and focus-on at the same time, which definitely would be an error.

The only case I was concerned about in the review is one where somebody wants to linger on a state across a few triggers:

:::{focus-on="pink" transition="fade-left" highlight-lines="1-4"}
Look at this key phrase.
:::

:::{}
It's really important, and we should think about it.
:::

:::{focus-on="pink" highlight-lines="5-8"}
Now, further down...
:::

Does the second block need to have the state from above duplicated, or can we scan back up to inherit it from above?

jimjam-slam commented 5 months ago

I should also say that I think transitions other than fade-in should probably be a post-release feature — I like them conceptually, but ones that involve translation might need some thought to avoid collisions with the scaling system 🤔

andrewpbray commented 5 months ago

I don't think we'd have a problem with your three div example using the partial implementation on rename-steps. Right now, the first Div would:

  1. Pull off any previous classes (like .cr-active and .cr-highlight)
  2. Add cr-active to "pink"
  3. Add highlighting classes to pink and the lines of pink

The second Div, since it doesn't have a focus-on or change-to attribute wouldn't trigger any of the above 3 actions, so the sticky should stay the same.

The third Div should:

  1. Pull off any previous classes
  2. Add cr-active to pink
  3. Add highlighting classes to the new lines of pink

And agreed about the alternative transitions - those can wait!

jimjam-slam commented 5 months ago

The second Div is more of a problem if someone triggers it from below, or scrolls quickly to it (missing the above one), or uses a section link to load the page directly to it, though 🤔

andrewpbray commented 5 months ago

Oh, you're totally right. That's a good argument for having the state of sticky-col defined for any place on the narrative the scroller tends to be.

The first fix you mentioned - copying the attributes from a valid step narrative block to any attribute-less narrative blocks that follow it - seems like a straightforward way to do it to me! This could be done in the Lua filter, but maybe best to do it in JS to make the functionality of closeread as similar as possible if in the future someone is using it without Quarto / Lua.

jimjam-slam commented 5 months ago

I think it should still be possible even with ID-based state (rather than index-based)! We have the list of trigger blocks, so we can locate the current one by its ID and step back from there.

andrewpbray commented 5 months ago

How are you thinking of triggering scrollama? On rename-steps, stepSelector currently looks for change-on or focus-on. Are you thinking of making the stepSelector trigger on every Para in the sidebar?

andrewpbray commented 4 months ago

Addressed in https://github.com/qmd-lab/closeread/pull/33.