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

reduce paragraph margin #704

Closed
wants to merge 1 commit into from
Closed

reduce paragraph margin #704

wants to merge 1 commit into from

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Oct 29, 2024

We previously bumped the paragraph margin to 1lh (#541), but having sat with it a while I think that was a mistake. We've had feedback — which I agree with — that the relationship between headings and copy is unclear. It also results in lower-than-ideal information density.

While 1lh is considered something of a 'rule' in typography, I do think this works better in basically all cases (old on the left, new on the right):

image image image image image

@@ -45,7 +45,7 @@ p,
ul,
ol {
font: var(--sk-font-body);
margin: 1lh 0;
margin: 0.5lh 0;
Copy link
Member

Choose a reason for hiding this comment

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

I agree that 1lh seems like a lot and we could do less. it does feel a little close together now, but not by much. what do you think about giving it just a tad more at 0.6lh?

Suggested change
margin: 0.5lh 0;
margin: 0.6lh 0;

Copy link
Member Author

Choose a reason for hiding this comment

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

Typography people like to talk about 'vertical rhythm' — that's where the 1lh came from in the first place. We're breaking that rule in a lot of places (we'd need to build the whole thing from scratch rather than merely changing the old site if we wanted to be really serious about it) but I do think that 'things should be multiples of each other' is sage advice that we should adhere to:

image

With 0.6lh, everything becomes... bumpy. Just very slightly off:

image

@benmccann
Copy link
Member

I think we could also reduce the margin between headings. E.g. the margin-top on h2 could go from 7rem to maybe 5rem

After that, we could potentially use some of the reclaimed vertical space to tweak the line-height on small screens where Georgia is used, which currently feels quite scrunched. It feels a bit more comfortable to me bumping it from 1.5 to 1.65

@Rich-Harris
Copy link
Member Author

tweak the line-height on small screens where Georgia is used, which currently feels quite scrunched

That was recently changed at @dummdidumm's behest

@dummdidumm
Copy link
Member

Have you seen #698? Takes a more complete approach to this; there's a few other places where spacing tweaks are sorely needed

@dummdidumm
Copy link
Member

After that, we could potentially use some of the reclaimed vertical space to tweak the line-height on small screens where Georgia is used, which currently feels quite scrunched

I can't go back to what it was before, it was all swimmy loosy-goosy with so much line-height

@Rich-Harris
Copy link
Member Author

I'm looking at it now, yeah. I think there's good stuff in there but 0.5lh works better, looking at them side-by-side (and also for the reasons given above) — in general I think we should probably prefer to use lh units than rem for typography (as opposed to layout). I'll make suggestions on that PR

@Rich-Harris
Copy link
Member Author

closing in favour of #698

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.

3 participants