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

πŸ‘©πŸ»β€πŸŽ¨ Improve margins and spacing in grids #77

Merged
merged 1 commit into from
May 11, 2023

Conversation

rowanc1
Copy link
Collaborator

@rowanc1 rowanc1 commented May 11, 2023

Originally created by @tavin in #76

This tightens up margins and simulates margin collapse in a grid by eating the top margins. This works inside and outside of the grid, while also allowing us to position child content in the margins.

Thanks @tavin for getting the ball rolling here, sorry to step on your PR. I will get this out in the next day or so as a theme-release!

Examples / Comparisons

image

image

Comment on lines 9 to 14
.article-grid > * {
/* The default is spanning the body for any child component */
@apply col-body;
/* Grids do not have margin-collapse, so each direct child needs to be addressed */
margin-top: 0 !important;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tavin, this is basically the fix. The rest is making sure that callouts/equations are balanced and have the same margins as paragraphs.

Copy link
Contributor

Choose a reason for hiding this comment

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

understood πŸ‘

@rowanc1 rowanc1 merged commit a61aa03 into main May 11, 2023
@rowanc1 rowanc1 deleted the fix/margins branch May 11, 2023 17:42
@@ -289,7 +289,7 @@ export function FrontmatterBlock({
subject || github || venue || biblio || open_access || license || hasExports || isJupyter;
const hasDateOrDoi = doi || date;
return (
<>
<div className="mb-8">
Copy link
Contributor

Choose a reason for hiding this comment

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

May I just ask about this mb-8 here? It's giving me 2rem extra space that wasn't there before -- between the title and content, or else just pushing the content down when there's no title.

If I remove the mb-8 class everything looks perfect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes, if you only have a title this isn't going to be good. I was testing with a full frontmatter, and that should be on conditionally (or something). Good catch!

We want these conditionally if there is frontmatter. Or maybe better is to override the h1 here and have an explicit mb-0 and then have some margin around the stuff that comes after it which is conditionally rendered.

Are you in a place to open a PR for this? If not, I can fix my bug shortly!

The two green things stack, and they should collapse if there isn't anything in between.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I guess I need to think about it a bit. You may get there faster than me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I am not quite sure which solution is best. I will try and get to it by the end of today and tag you! Thanks again for pointing it out. :)

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.

None yet

2 participants