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.

Sign up for GitHub

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

Jump to bottom

Fix line height spacing for multiline code elements #162

Merged
merged 3 commits into from
Nov 25, 2019
Merged

Fix line height spacing for multiline code elements #162

merged 3 commits into from
Nov 25, 2019

Conversation

tombye
Copy link
Contributor

@tombye tombye commented Nov 15, 2019

Text in our <code> elements is larger than the default Transport font on OSX, due to it using the Monaco font. This creates a taller formatting box for each line of text it has.

We set a smaller font-size, and line-height in <td>s. This works for Transport but text in <code> sections appears squashed vertically.

This sets the line-height for <code> to match that of content outside tables, to stop it
squashing up.

This has been tested with Consolas (the font Windows uses for <code>) and all other
fallbacks.

Here's an idea of the changes this introduces, viewed on OSX Firefox.

Before

image

After

image

@tombye tombye mentioned this pull request Nov 15, 2019
Closed
NickColley
NickColley approved these changes Nov 18, 2019
View reviewed changes
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Needs a CHANGELOG entry

lib/assets/stylesheets/modules/_technical-documentation.scss
@@ -141,6 +141,8 @@
font-family: monaco, Consolas, "Lucida Console", monospace;
font-size: 15px;
font-size: 0.9375rem;
// Match the line-height outside of tables
line-height: 1.4;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the keyword normal would work, but happy with this if not.

Copy link
Contributor Author

@tombye tombye Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I tested this and it would work for all the monospaced fonts we specify if we didn't have the padding on the top and bottom of the line. With that, normal still causes some vertical overlap, though it depends on which browser how much.

NickColley
NickColley reviewed Nov 18, 2019
View reviewed changes
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

What impact does this have to code elements within paragraphs?

@NickColley
Copy link
Contributor

I wonder if this is related: #131

@tombye
Copy link
Contributor Author

tombye commented Nov 19, 2019

What impact does this have to code elements within paragraphs?

It seems to work OK:

image

(from Notify's REST API docs.)

Looks like the line-height leaves enough space for the padding on the top and bottom.

@NickColley
Copy link
Contributor

NickColley commented Nov 19, 2019 •
edited
Loading

I'm still wondering if the reason this has broken is because the relative font units is not working in that area for some reason 🤔

The text looks a lot bigger than the surrounding regular text.

@NickColley
Copy link
Contributor

Would you mind adding an example to: http://127.0.0.1:4567/code.html#code-examples please?

tombye added a commit that referenced this pull request Nov 19, 2019
@tombye
Add CHANGELOG entry for pull request #162
87f1285
@tombye
Copy link
Contributor Author

tombye commented Nov 21, 2019

I'm still wondering if the reason this has broken is because the relative font units is not working in that area for some reason 🤔

The text looks a lot bigger than the surrounding regular text.

On smaller screens, the <code> is larger than the normal table text, 15px vs 14px, whatever the monospace font-family. On larger screens, they're both 16px.

I did some testing on the differences in vertical size between the monospace font-families and New Transport.

I measured the height from the ascender to the descender on various combinations, all at 16px, and found:

  • Monaco and New Transport at 16px were roughly the same
  • Consolas and New Transport showed some difference with Consolas being about 3px smaller
  • monospace was the same as Consolas

All was on Firefox on OSX.

@NickColley NickColley self-assigned this Nov 21, 2019
@NickColley
Copy link
Contributor

I'll try and review this when I get some free time, please ping me if I don't.

@tombye
Copy link
Contributor Author

tombye commented Nov 21, 2019

I'll try and review this when I get some free time, please ping me if I don't.

Will do 👍

NickColley
NickColley reviewed Nov 25, 2019
View reviewed changes
CHANGELOG.md
@@ -2,6 +2,13 @@

## Unreleased

## 2.0.10
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs rebasing and adding to the 'fixes' section of the unreleased stuff

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to do this for you if you want

Copy link
Contributor

Choose a reason for hiding this comment

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

Tom said he was happy for me to get this sorted

@tombye @NickColley
Increase line-height for <code> in <td>s
fc2afbe 
Text in our `<code>` elements is larger than the
default Transport font on OSX, due to it using
the Monaco font. This creates a taller formatting
box for each line of text it has.

We set a smaller font-size, and line-height in
`<td>`s. This works for Transport but text
in `<code>` sections appears squashed vertically.

This sets the line-height for `<code>` to match
that of content outside tables, to stop it
squashing up.

This has been tested with Consolas (the font
Windows uses for `<code>`) and all other
fallbacks.
@NickColley NickColley changed the title Increase line-height for <code> in <td>s Fix line height spacing for multiline code elements Nov 25, 2019
@NickColley NickColley force-pushed the increase-table-cell-code-line-height branch from ea6b425 to 31f551f Compare November 25, 2019 15:22
@NickColley NickColley merged commit 8ba6de9 into master Nov 25, 2019
@NickColley NickColley deleted the increase-table-cell-code-line-height branch November 25, 2019 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NickColley NickColley NickColley approved these changes

Assignees

@NickColley NickColley

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tombye @NickColley