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

Overlay and sidebar layout options #55

Merged
merged 37 commits into from
Jul 31, 2024
Merged

Conversation

jimjam-slam
Copy link
Collaborator

@jimjam-slam jimjam-slam commented Jul 29, 2024

Closing #24 in favour of this (I also cherry-picked the commits from #52 and #53, as they generalise the ` injection code. Shouldn't matter which is merged first!). Adds two options:

format:
  closeread-html:
    overlay-side: # "left", "center or "right"
    sidebar-side: # "left" or "right"

The Lua filter throws an error if both are used on the same doc. These options set a class on <body>.

These classes can be be inherited on individual .cr-layout using helper classes:

  • .cr-layout-overlay-left
  • .cr-layout-overlay-center
  • .cr-layout-overlay-right
  • .cr-layout-sidebar-left
  • .cr-layout-sidebar-right

What do you think, @andrewpbray? I haven't touched colours here, just layout! I also didn't try to do tablet layouts: the layout locks for mobile devices (< 576 px, equivalent to Bootstrap's XS class), but anything wider than that uses the same layout. Just wanted to make sure I got this done before I took off! 😅

Also note I've hard coded the margins for now, but we could potentially write in additional options to pass that in from YAML. For now I've set the sidebar columns ratios to 1fr 2fr, overlay margins to 65%/5% (or 5%/65% on the left). Centre margins are auto, but perhaps we want to set those to percentages too to fix the width. Mobile narrative margins are 0.5em.

@jimjam-slam
Copy link
Collaborator Author

Re. sidebar widths, we may want to adjust this to accommodate your comment @andrewpbray:

While tinkering around with styling, I realized nytimes keeps their sidebar at a fixed width. Try widening and narrowing the window here:

https://www.nytimes.com/interactive/2022/03/06/books/auden-musee-des-beaux-arts.html

@andrewpbray
Copy link
Contributor

andrewpbray commented Jul 29, 2024

Ooo, feel like a kid in a candy shop getting to look at all this fine work! This inspires three lines of thinking:

YAML [1/3]
On a very long roadtrip yesterday I got to thinking about how we could leverage logic similar to code cell options for propagating yaml options down into our document. What are your thoughts on having the users do something like:

format:
  closeread-html:
    cr-section:
      layout: {sidebar-left, overlay-center, etc}
    trigger:
      padding: 40svh
      propagate-focus: {up, down, false}
    sticky:
      margin: 10 10 10 10 

The propagate-focus key could be the lua filter behavior that we spoke of yesterday but the remainder would be ways to pass stuff to the classes .cr-section, .trigger, or .sticky (I'd imagine just offering up the most-used style attributes for those who don't want to tinker with the .css file). In the document, we could allow syntax like:

:::{.cr-section layout="overlay-right"}
:::

Which the filter would annotate with a higher level of specificity to override any global layout specified in the yaml.

@andrewpbray
Copy link
Contributor

andrewpbray commented Jul 29, 2024

JS vs Lua [2/3]
What's the rationale behind assigning some of these classes in the js instead of in the Lua filter? My rough model for the division of responsibilities there is that the Lua makes the HTML doc (and css) that populates the initial doc that loads. Any changes to it based on user input (i.e. scrolling) is handled by js.

Would it work to write a lua filter that would look something like:

-- set default
local section_layout = "sidebar-left"
-- overwrite section_layout if found in yaml via separate filter on meta

function assign_layouts(div)
  
  if div.classes:includes("cr-section") then
    -- loop over attributes to find any that are layout
    --  if found, use it to overwrite section_layout
  
    -- form appropriate class string from section_layout
    -- add class to div and return it
   
  end
end

What are your thoughts on the tradeoffs of these two methods? I know your time is very limited. I'm happy to work on a lua implementation if that's helpful.

@andrewpbray
Copy link
Contributor

Display [3/3]

I'm running into some unexpected behavior when viewing https://closeread.netlify.app/, which published via quarto publish locally.

Sticky blocks looks like the overlay layout, as planned (albeit with some ojs errors). Minard, though, looks like it's still on sidebar-right:

Screenshot 2024-07-29 at 12 41 08 PM

Any ideas what's up?

To loop back to this topic:

Re. sidebar widths, we may want to adjust this to accommodate your comment @andrewpbray:

While tinkering around with styling, I realized nytimes keeps their sidebar at a fixed width. Try widening and narrowing the window here:

It's not a super strong preference, but I think I do prefer that fixed sidebar. Right now nytimes.css is getting there with min-width: 400px; which... I don't quite understand. It seems like it's behaving as if width: 400px, i.e. it does grow beyond 400 px when the page gets very wide.

What do you think the best way is to implement this? Which units?

@@ -0,0 +1 @@
{"version":3,"sourceRoot":"","sources":["grid.scss"],"names":[],"mappings":"AAAA;AAEA;EACE;EACA;;AAEA;EACE;;AAEA;EAEE;EACA;;AAEA;EACE;;AAKN;EACE;;AAIA;EACE;EACA;EACA;EACA;EACA;;AAEA;EACE;EACA;EAGA;EAEA,YACE;;AAOJ;EACE;;;AAMR;AACA;EAEI;IACE;;EAEA;IACE;IACA;IACA;;EAGF;IACE;;;AAMR;AAAA;AAAA;AAAA;AAMA;AAAA;AAAA;AAAA;AAAA;AAAA;EAOE;;AAEA;AAAA;AAAA;AAAA;AAAA;AAAA;EACE;EACA;;AAGF;AAAA;AAAA;AAAA;AAAA;AAAA;EACE;;;AAOF;AAAA;EACE;EACA;;;AAKF;AAAA;EACE;EACA;;;AAKF;AAAA;EACE;;;AAKJ;AAAA;EAGE;;AAEA;AAAA;EACE;EACA;;AAGF;AAAA;EACE;;;AAGJ;AAAA;EAGE;;AAEA;AAAA;EACE;EACA;;AAGF;AAAA;EACE;;;AAIJ;AAEA;EAEE;AAEA;AAAA;EAEA;EACA;EAEA;EACA;EAGA;;AAGA;EACE;;AAGF;EACE;EACA;;;AAKJ;AAKE;EACE;EACA;;AAIF;EACE;;AAIF;EACE;EACA;;AAEA;EACE;EACA;EACA;;;AAMN;AAII;EACE;EACA;;AAEA;EACE","file":"grid.css"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're including the css and the scss files in the extension, what's the role of this map file (I've never seen these before)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't worked with them much, but my understanding is that they allow someone using the browser developer tools to look up a stylesheet rule that is applied to an element (mochas we were doing the other day, clicking on bootstrap.min.css next to a rule) and be redirected to the SCSS that generated it. So it maps the SCSS to the compiled CSS.

That said, it's not necessary for the user! If we wanted, we could git rm the map and then put it on .gitignore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'd be inclined to git rm it since I'd bet anyone whose doing that will be able to figure out where to look in these stylesheets.

_extensions/closeread/scroller-init.js Outdated Show resolved Hide resolved

// overlay layouts use one column...
.cr-default-overlay-left .cr-layout,
body .cr-layout.cr-overlay-left,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is body included here just to make the level of specificity high enough to override the .cr-default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. You'll see with the mobile sizing I also had to pop an additional class, .closeread, on the <body> purely to make the specificity work (otherwise the mobile sizing was overridden by user preference).

If we go with your approach of ensuring in Lua that every .cr-layout is assigned a layout class, none of this will be needed (we'll still need to be more specific with mobile sizing, but that because much easier).


/* extended wrapper around content to allow early scroll detection */
grid-row: 1;

> * {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can be more precise with this selector if we use .narrative-block (see convo in OJS demo PR #40 ).

_extensions/closeread/grid.scss Outdated Show resolved Hide resolved
@@ -61,13 +52,111 @@
}
}

/* mobile sizing (bootstrap: xs) is always overlay-center */
@media (max-width: 575.98px) {
body.closeread.cr-default-overlay-right {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be body.closeread.cr-default-overlay-center {?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! I originally had body.closeread[class^=cr-default] to encompass all five of the layout classes, but it wasn't sticking and I couldn't work out why... I wrote this one as a test case and then forgot to generalise it again. If we absolutely must, we can just write out all five manually:

@media (max-width: 575.98px) {
  body.closeread.cr-default-overlay-left,
  body.closeread.cr-default-overlay-center,
  body.closeread.cr-default-overlay-right,
  body.closeread.cr-default-sidebar-left,
  body.closeread.cr-default-sidebar-right {

Basically it's that regardless of the layout class, on mobile it should behave as overlay-centre. The rest of it is there for specificity. Again, this becomes much easier if we take your approach of assigning the layout classes in Lua 😊

// using a grid to stack sticky elements on top of each other to then
// transition through (based on reveal's .r-stack)
.sticky-col-stack {
display: grid;
height: 100svh;
height: 100dvh;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you determining when you use dvh and when to use svh? The little I've read about them seem to warn away from dvh.

@jimjam-slam
Copy link
Collaborator Author

JS vs Lua [2/3] What's the rationale behind assigning some of these classes in the js instead of in the Lua filter? My rough model for the division of responsibilities there is that the Lua makes the HTML doc (and css) that populates the initial doc that loads. Any changes to it based on user input (i.e. scrolling) is handled by js.

Would it work to write a lua filter that would look something like:

-- set default
local section_layout = "sidebar-left"
-- overwrite section_layout if found in yaml via separate filter on meta

function assign_layouts(div)
  
  if div.classes:includes("cr-section") then
    -- loop over attributes to find any that are layout
    --  if found, use it to overwrite section_layout
  
    -- form appropriate class string from section_layout
    -- add class to div and return it
   
  end
end

What are your thoughts on the tradeoffs of these two methods?

If you're up for writing Lua to assign either the default layout class (according to the YAML or a fallback default) or the specified class on each .cr-layout, I think that would simplify the CSS a lot and remove a good bit of JS. I'd love to see that!

My Lua's gotten a bit rusty, so when I approached this I went with the technique I'd used last week for the "remove header space" work: writing from the YAML to a <meta> tag, and then transferring that to the body in the JS (as much as I don't love that technique). Though now you lay it out in this comment, it doesn't seem as difficult as it felt yesterday 😂

I know your time is very limited. I'm happy to work on a lua implementation if that's helpful.

I appreciate that, and I'm sorry I can't hang around to refine this further! I was pushing this work around my plate a bit 😅

@jimjam-slam
Copy link
Collaborator Author

Ooo, feel like a kid in a candy shop getting to look at all this fine work! This inspires three lines of thinking:

YAML [1/3] On a very long roadtrip yesterday I got to thinking about how we could leverage logic similar to code cell options for propagating yaml options down into our document. What are your thoughts on having the users do something like:

format:
  closeread-html:
    cr-section:
      layout: {sidebar-left, overlay-center, etc}
    trigger:
      padding: 40svh
      propagate-focus: {up, down, false}
    sticky:
      margin: 10 10 10 10 

The propagate-focus key could be the lua filter behavior that we spoke of yesterday but the remainder would be ways to pass stuff to the classes .cr-section, .trigger, or .sticky (I'd imagine just offering up the most-used style attributes for those who don't want to tinker with the .css file). In the document, we could allow syntax like:

:::{.cr-section layout="overlay-right"}
:::

Which the filter would annotate with a higher level of specificity to override any global layout specified in the yaml.

I like this! If we add additional options to set widths and such, I imagine we'd write them directly onto the elements' style attributes and strip them out of the SCSS.

@jimjam-slam
Copy link
Collaborator Author

Display [3/3]

I'm running into some unexpected behavior when viewing https://closeread.netlify.app/, which published via quarto publish locally.

Sticky blocks looks like the overlay layout, as planned (albeit with some ojs errors). Minard, though, looks like it's still on sidebar-right:

Screenshot 2024-07-29 at 12 41 08 PM Any ideas what's up?

Is this happening on the main branch, or on a branch deploy for this branch? Minard seemed to work well with all the layouts when I tested it locally... 🤔

To loop back to this topic:

Re. sidebar widths, we may want to adjust this to accommodate your comment @andrewpbray:

While tinkering around with styling, I realized nytimes keeps their sidebar at a fixed width. Try widening and narrowing the window here:

It's not a super strong preference, but I think I do prefer that fixed sidebar. Right now nytimes.css is getting there with min-width: 400px; which... I don't quite understand. It seems like it's behaving as if width: 400px, i.e. it does grow beyond 400 px when the page gets very wide.

What do you think the best way is to implement this? Which units?

Instead of using grid here, they're using a flexbox. The advantage of a flexbox is that you can specify the degree to which elements can grow or shrink to use up additional available space.

I haven't used it before, but we might be able to replace 1fr for the sidebar with minmax(400px, 1fr) to enforce a minimum. That said, if we do we might want to bump our mobile sizing threshold up from XS: 576px leaves just 176px for the content in the worst case. The next Bootstrap breakpoint up is 768px, which leaves 368px for the content.

But I think if someone was using an overlay layout, they'd probably want a narrower switch. So maybe we should have separate minimums for sidebar and overlay (not hard to do at all).

@andrewpbray
Copy link
Contributor

Display

Is this happening on the main branch, or on a branch deploy for this branch? Minard seemed to work well with all the layouts when I tested it locally... 🤔

That was on the feature-layout-options branch when published from my local machine to netlify. I just published another version from that branch to my quarto pub account, but it looks the same: https://andrew.quarto.pub/closeread3/. Do you want to try publishing to quarto pub and see how it looks?

I haven't used it before, but we might be able to replace 1fr for the sidebar with minmax(400px, 1fr) to enforce a minimum.

Sounds good to me! We can tinker with it as we build out the gallery and doing more fine tune testing as the talk approaches. (speaking of talk, I'll be talking about this for the first time at JSM in about a week. That'll be a good dry run for the posit::conf.)

YAML syntax and JS vs Lua
I'll go ahead and work on this implementation! 🚀

One thing about your approach of meta tag injection: I think that might be necessary since it needs to be attached to <body>. The stuff for the sections is all accessible in the AST, though, so it should be straightforward. 🤞

@jimjam-slam
Copy link
Collaborator Author

That sounds good!

Do you want to try publishing to quarto pub and see how it looks?

I can't make promises, I'm afraid 😔 I might be able to later in the week if I find myself with some down time, but the next couple days I'll be in the air!

@andrewpbray
Copy link
Contributor

Here's the kernel of the approach of reading layout from the document yaml but allowing a section attribute to override it: a6698c0.

That was quite a bit simpler than I imagined!

@andrewpbray andrewpbray mentioned this pull request Jul 31, 2024
@andrewpbray andrewpbray merged commit 6cf9bec into main Jul 31, 2024
1 check passed
@andrewpbray andrewpbray deleted the feature-layout-options branch July 31, 2024 17:40
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