search

qmd-lab / closeread

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

bugfix: figure env messes up grid #82

Closed andrewpbray closed 3 weeks ago

andrewpbray commented 3 months ago

Fixes #81

andrewpbray commented 2 months ago

We do indeed add the .column-screen class!

https://github.com/qmd-lab/closeread/blob/d5583d1c41abd2eb3f8673b403b522c178dbb537/_extensions/closeread/closeread.lua#L77

jimjam-slam commented 2 months ago

Are we adding it to the image itself, or just to the section? When I inject logging statements, it seems like we're not adding it to the image, but it's ending up there nonetheless... maybe I need to take another look!

EDIT: I'll keep the discussion over on #81 so I don't get us tangled, but I tentatively think it's not us! Will let you digest my thoughts over there before I go further though 😅

andrewpbray commented 2 months ago

This isn't the solution to the problem, but while looking at the Lua I found this at the very bottom of the file

https://github.com/qmd-lab/closeread/blob/d5583d1c41abd2eb3f8673b403b522c178dbb537/_extensions/closeread/closeread.lua#L519

As far as I can tell, I don't think that's actually a function that we define?

I'm just recording that here for now.

jimjam-slam commented 1 month ago

With https://github.com/qmd-lab/closeread/pull/82/commits/f1eb9b24b1a197a5dc08a2d08fc24fd6e3555f6e, this is ready for review to merge in @andrewpbray! The change to propagate classes back is based on the similar Quarto code

andrewpbray commented 1 month ago

Oh hey: since this was originally into main, looks like we get that action:

https://closeread-pr.netlify.app/gallery/demos/sticky-blocks/

Looks like it's not fixed there...hmm... what's going on here.

Sidenote: sticky_blocks.qmd showing up makes me think: do we want to showcase all of these tests as "Demos" or is there some tension there that would be better resolved using Demos for clear functionality vs some other form of less public test doc?

jimjam-slam commented 1 month ago

Oh hey: since this was originally into main, looks like we get that action:

https://closeread-pr.netlify.app/gallery/demos/sticky-blocks/

Looks like it's not fixed there...hmm... what's going on here.

Are our actions deploying on push to a branch or on opening a PR into a branch? I'm not sure exactly when pull_request actions fire with respect to a PR! We probably also don't want every PR opened into main overwriting our published production docs either...

Sidenote: sticky_blocks.qmd showing up makes me think: do we want to showcase all of these tests as "Demos" or is there some tension there that would be better resolved using Demos for clear functionality vs some other form of less public test doc?

Mmm, I do wonder if we'd be better cordoning off tests. They could potentially stay on the site if it's easiest to keep them there (to take advantage of our current CI/CD), but maybe putting them in a separate category and labelling them as being for internal testing purposes is a good idea (once the work is fully merged in).

jimjam-slam commented 1 month ago

Have stopped going all the way up the DOM in https://github.com/qmd-lab/closeread/pull/82/commits/c9c6a9b673d6e3144c5ca6b4244e45b6144fb197. Returning to the sticky blocks demo, we do still have a problem. Our style stretching it across the screen is being overwritten by a more specific one from Quarto:

.page-columns.column-page-left .page-columns.page-full>*,        // this is the one that's triggering
.page-columns.column-page-left>* {
  grid-column: page-start/body-content-end;
}

I think the easiest way around this is probably just to set our CSS grid style as !important, which should override just about anything Quarto sets regardless of the specific page layout Quarto is using. This will change as we decide how to accommodate sidebars, but I think it makes to evaluate whether to change this style as part of that work rather than here.

jimjam-slam commented 1 month ago

(When we merge this one, we should probably squash it! There's been a lot of back-and-forth in the commits 😅)

andrewpbray commented 1 month ago

It feels like this is one of those rare cases when !important really is called for!

In terms of whether sticky blocks should be a demo or a test, I agree with your thoughts there. If we don't cordon off tests, among other things, it will bloat the docs with stuff that users might be kinda like wtf on. How about this approach for testing (just spitballing...):

  1. Put any test qmd files in docs/tests
  2. Add project.render["*.qmd", "!tests"] to _quarto.yml
  3. Create a new _quarto-tests.yml file with project.render["tests"]
  4. In both actions, add an additional step before quarto publish where we call quarto render --profile tests.

When the profile metadata gets merged in, hopefully the "tests" glob overwrites the "!tests" one?

andrewpbray commented 3 weeks ago

🎉 🎉 🎉