Skip to content

Commit

Permalink
force txt-bold to be 1em line height, fixes line height offsets, closes
Browse files Browse the repository at this point in the history 
#775
  • Loading branch information
@samanpwbb
samanpwbb committed May 25, 2017
1 parent 8a97b51 commit b2f9b57
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 11 deletions.
22 changes: 13 additions & 9 deletions src/typography.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,19 @@ textarea {
font-size: 90%; /* mono fonts have naturally large proportions, scale them down to match open sans. */
}

/**
* Apply a bold font weight.
*
* @memberof Type utils
* @example
* <div class='txt-bold'>txt-bold</div>
*/
.txt-bold {
font-weight: bold !important;
/* prevent bold text inside inline text from offsetting line height */

This comment has been minimized.

Sign in to view

Copy link
@davidtheclark

davidtheclark May 27, 2017

Contributor

This seems like a very strange choice to me, @samanpwbb. Can you explain why it works the way you want it to? Does it have strange side-effects? — e.g. if you have multi-line bold text next to multi-line non-bold text of the same size?

This comment has been minimized.

Sign in to view

Copy link
@samanpwbb

samanpwbb May 27, 2017

Author Contributor

This works in all the cases I can find where line heights were previously being offset. It doesn't appear to have any strange side effects.

There's some background in https://stackoverflow.com/a/8376913/6836364 – I am not entirely sure why this solution works. Seems like a inline element next to text (or other inline elements) has it's line-height computed separately from parent context, and this causes problems when using switching fonts. The fix forces the line height of the inner span to be smaller than it would be otherwise.

This comment has been minimized.

Sign in to view

Copy link
@davidtheclark

davidtheclark May 27, 2017 •
edited
Loading

Contributor

Here's the side-effect that came to my mind and I just, sadly, confirmed:

screen shot 2017-05-27 at 3 44 26 pm

Multi-line bold text has a shorter line-height than non-bold text.

This comment has been minimized.

Sign in to view

Copy link
@samanpwbb

samanpwbb May 28, 2017

Author Contributor

In this case the text could either have a txt- rule or be in the context of a txt- parent element, which should fix this problem right?

This comment has been minimized.

Sign in to view

Copy link
@davidtheclark

davidtheclark May 28, 2017

Contributor

I'm afraid I don't really understand what you're suggesting.

This comment has been minimized.

Sign in to view

Copy link
@samanpwbb

samanpwbb May 28, 2017 •
edited
Loading

Author Contributor

The case you brought up seems uncompon so we should not revert this change. Bold without a corresponding txt- rule shouldn't happen

This comment has been minimized.

Sign in to view

Copy link
@davidtheclark

davidtheclark May 28, 2017

Contributor

Bold without a corresponding txt- rule shouldn't happen

I still don't understand what you mean. In the example I'm using txt-bold — are you referring to some other rule? Or .prose?

Here's another illustration of the problem:
screen shot 2017-05-28 at 4 43 04 pm

Here's the corresponding code:

<div className='mb12'>
  <div className='inline-block bg-red px12 py6 txt-bold'>
    active
  </div>
  <div className='inline-block bg-pink px12 py6'>
    not active
  </div>
</div>

This comment has been minimized.

Sign in to view

Copy link
@samanpwbb

samanpwbb May 29, 2017

Author Contributor

Hm I thought about this some more while walking along the beach this morning. I now agree with @davidtheclark that we should revert this change. In practice, the change introduces benefits as well as potential pitfalls. But at the end of the day unless we can guarantee zero side effects, utility classes should not include unexpected style declarations.

The only reliable way to fix this issue is to use a different font. Until then, we should provide a suggested workaround for bold text inside a block of text: either by adding a line-height: 1em utility class, or with another recommendation.
line-height: 1em;
}

/**
* Classes for font size and line height, lists, block quotes, code blocks, and more.
*
Expand Down Expand Up @@ -376,15 +389,6 @@ textarea {
font-weight: normal !important;
}

/**
* Apply a bold font weight.
*
* @memberof Type utils
* @example
* <div class='txt-bold'>txt-bold</div>
*/
.txt-bold { font-weight: bold !important; }

/**
* Italicize text.
*
Expand Down
10 changes: 8 additions & 2 deletions test/__snapshots__/build-css.jest.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,10 @@ textarea{
font-family:'Menlo', 'Bitstream Vera Sans Mono', 'Monaco', 'Consolas', monospace;
font-size:90%;
}
.txt-bold{
font-weight:bold !important;
line-height:1em;
}
.txt-h1{
font-size:45px;
line-height:54px;
Expand Down Expand Up @@ -393,7 +397,6 @@ textarea{
.txt-normal{
font-weight:normal !important;
}
.txt-bold{ font-weight:bold !important; }
.txt-em{ font-style:italic !important; }
.txt-uppercase{ text-transform:uppercase !important; }
.txt-lowercase{ text-transform:lowercase !important; }
Expand Down Expand Up @@ -16732,6 +16735,10 @@ textarea{
font-family:'Menlo', 'Bitstream Vera Sans Mono', 'Monaco', 'Consolas', monospace;
font-size:90%;
}
.txt-bold{
font-weight:bold !important;
line-height:1em;
}
.txt-h1{
font-size:45px;
line-height:54px;
Expand Down Expand Up @@ -16861,7 +16868,6 @@ textarea{
.txt-normal{
font-weight:normal !important;
}
.txt-bold{ font-weight:bold !important; }
.txt-em{ font-style:italic !important; }
.txt-uppercase{ text-transform:uppercase !important; }
.txt-lowercase{ text-transform:lowercase !important; }
Expand Down

0 comments on commit b2f9b57

Please sign in to comment.