qmd-lab / closeread

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

Rename steps #33

Closed andrewpbray closed 4 months ago

andrewpbray commented 5 months ago

Change syntax around transitions and focus steps to simplify markdown source and JS code. Examples of new syntax:

{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”}

jimjam-slam commented 5 months ago

Have just tacked a few commits on here (2d3a104, 176dcb4 and a2dd539) to:

jimjam-slam commented 5 months ago

@andrewpbray I like the switch to using IDs instead of indices generally! I wasn't sure if this is a draft PR, since it seems like the rescaling functionality is turned off at the moment, with the window resize handler disabled and updateStickies removed from the scroll handlers.

andrewpbray commented 5 months ago

What is a draft PR? I think the answer to that question, though, is yes =). That's to say, I wanted to try the indexless approach while keeping the other code around for reference. I also wasn't quite sure what to do with the rescaling. That had been packaged up with the highlighting, , but now that highlighting is pulled out, think it'll work as a separate function?

I went back and looked at the WH Auden poem (and the way that you implemented both highlighting and zooming simulatenously). The effect of those two combined really is potent, so I guess the questions are:

  1. Are they fully separable operations that can live in different functions? (Pretty sure the answer is yes)
  2. What's the best syntax for the user to use when they want to involve both?
andrewpbray commented 5 months ago

It also occurred to me that on window resize, the only function we'll need to call is the rescaling stuff, right? (not the transitions/highlights, etc)

jimjam-slam commented 5 months ago
  1. I think so, if highlighting doesn't include zooming/translating to focus on a span!

  2. Good question!

on window resize, the only function we'll need to call is the rescaling stuff, right? (not the transitions/highlights, etc)

I think so! Although a resize may cause the narrative content in view to change, and I'm not sure whether that causes their trigger code to run. If not, it may need to after all. It can probably wait until a resize end event if one is available though!

andrewpbray commented 5 months ago

I definitely let scope-creep take over this PR. It was meant to be just swapping out the step names (eliminating the from) and grew to include both removing the indices from the sticky activating system and refactoring the highlighting code. I'd like to tie this guy off before he gets more convoluted.

Right now, there's no zooming functionality on this branch, so merging it will be a regression in the functionality of demo-block-types. What do you think the best way to restore that functionality?

jimjam-slam commented 5 months ago

Mmm, sorry, I jumped in on this one a bit too! I think once we're happy with #28 and #36, that might make things easier to align on and merge in.

andrewpbray commented 4 months ago

@jimjam-slam I'd like to put a bow on this so that we can move forward with the simpler step syntax of just using focus-on. The rub is that I foolishly started a quite different project in here: refactoring recalculateActiveSteps to split out zooming, highlighting, and swapping .cr-active in anticipation of expanding each of them for new block types. The main commit is https://github.com/qmd-lab/closeread/pull/33/commits/3c42e63f784002dbc9b8106f1666dab34f8915a3 and it succeeds in breaking the poem.

I'm happy to snip out that commit and reimplement elsewhere but I believe you may be using this version in the feature-ojs branch. What do say: snip out the commit or merge and fix with another PR?

jimjam-slam commented 4 months ago

I'm happy for you to keep going on this branch if you want! I can monitor and merge extra commits back over to feature-ojs if needed 😊

andrewpbray commented 4 months ago

@jimjam-slam A few of the threads that we've been working on - grid layouts, text zooming, and highlighting - are mixed together across a few branches. In an effort to isolate them, I propose:

  1. Merging thing branch into main knowing it'll break poem zooming.
  2. Create a zoom-to branch, where we work on restoring that functionality, either through the original transform approach or the adaptive font size approach (which I think is on the branch with the grid layouts?)
  3. Create a highlight branch where we can extend text highlighting to include code highlighting.

What do you think?

jimjam-slam commented 4 months ago

This sounds good to me! I should be on a bit later in the day!