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

bugfix: figure env messes up grid #82

Merged
merged 16 commits into from
Oct 30, 2024
Merged

bugfix: figure env messes up grid #82

merged 16 commits into from
Oct 30, 2024

Conversation

andrewpbray
Copy link
Contributor

  • adds a demo file to demonstrate (failed functionality)

Fixes #81

@jimjam-slam jimjam-slam self-assigned this Sep 24, 2024
@jimjam-slam jimjam-slam marked this pull request as draft September 24, 2024 06:50
@andrewpbray
Copy link
Contributor Author

We do indeed add the .column-screen class!

local quarto_layout = "column-screen" -- default

@jimjam-slam
Copy link
Collaborator

jimjam-slam commented Sep 24, 2024

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
Copy link
Contributor Author

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

Pandoc = add_classes_to_body

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
Copy link
Collaborator

With f1eb9b2, this is ready for review to merge in @andrewpbray! The change to propagate classes back is based on the similar Quarto code

@andrewpbray andrewpbray changed the base branch from main to dev October 18, 2024 03:24
@andrewpbray
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Collaborator

Have stopped going all the way up the DOM in c9c6a9b. 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
Copy link
Collaborator

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

@andrewpbray
Copy link
Contributor Author

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 andrewpbray marked this pull request as ready for review October 28, 2024 21:57
@jimjam-slam jimjam-slam merged commit 026d24f into dev Oct 30, 2024
1 check passed
@andrewpbray andrewpbray mentioned this pull request Oct 31, 2024
@andrewpbray
Copy link
Contributor Author

🎉 🎉 🎉

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.

Figure environment messes up css grid
2 participants