Closed jrubenoff closed 8 years ago
Can you talk more about the decision to add units to the global line height? I'm trying to square it with https://css-tricks.com/almanac/properties/l/line-height/#article-header-id-0.
Maybe we should discuss on Monday.
We use a set, predictable range of font sizes that are hardcoded into variables, so that rule doesn't really apply. If we had a bunch of exceptions in our codebase where we were styling elements in arbitrary font sizes and prioritized flexibility over predictability, that advice would make sense. The approach in this PR gives us greater control over the design.
We were already using set line heights for h1
, h2
, h3
, etc., so this just extends that pattern to the whole codebase.
On call:
Alright, removed the units from all the line-heights. 1.5
is a little too tight for our smaller sizes, so I adjusted the $lineHeightSmall-
and $unitlessLineHeightSmall-
variables accordingly. Also made a few more minor improvements to the style guide.
Here are my comments as I went through the changed files and previewed the site. I also added some line comments where appropriate.
I think a good next step would be to talk in-person (or video) before making any more changes.
Contrary to https://github.com/dobtco/screendoor-v2/pull/3523#issuecomment-216679696, adding lines of code does seem to be a trend: http://take.ms/hAGhF. This is my primary resistance to this PR.
Similarly, I'm not sure the 1.64 line height is worth the added complexity, the likelihood that it won't be applied consistently, and the added file size.
And while we're on the subject: these large, multi-issue PRs are really a nightmare to deal with. We've talked about this before; how can we avoid them in the future?
I found what I think is a page header alignment bug: http://take.ms/ZZYVS
I might have missed it... why are "dos" and "don'ts" screenshots, instead of live HTML? The screenshots look kinda crappy on my display... I think because they're non-retina?
This footer line-height bug still exists: http://take.ms/ctfCv
The sidebar has some behavior that we've tried to avoid in the past: http://take.ms/ktK3X. (This might be why: http://take.ms/USHFY)
The redesigned user block looks nice, but feels a little cramped. How about adding some more padding on the right side? http://take.ms/lripS
I don't share your concerns around the page_header
and sidebar_data
, but I made a few aesthetic changes to those components. I also reverted the line-height mixins.
@jrubenoff: there are some merge conflicts that need to be fixed, now that I merged the other footer line height PR and the list styles PR.
Can you do that, and then maybe write up a new summary for this PR? I'm having trouble actually fguring out what this changes.
Done and done
Here's my list of what components changed:
<dt>
smallerI'm gonna preview all of these and look at the changes. I also left some new inline comments.
Here's all I have for now:
<dt>
really be $fontSmallest
? I thought we should use it sparingly.Thanks for sticking with this... 😅
Manually set the line-height for navbar_badge
.
<dt>
s are $fontSmaller
... maybe you misread?
<dt>
s are $fontSmaller... maybe you misread?
Yeah, I did. But did they get smaller in this PR?
Yes, they were fontSmall
before. Harder to distinguish from the body text.
Closes #249, #241, #223, #204, and dobtco/screendoor-v2#3302
line-height
, and makes appropriate alignment adjustmentsAlso:
<dt>
is now bold for added clarity: