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

Typography tweaks for kbd and code in indicators #4400

Merged
merged 2 commits into from Sep 6, 2017

Conversation

Projects
None yet
3 participants
@stephaniehobson
Contributor

stephaniehobson commented Sep 1, 2017

Fix bug 1376770.

Testing notes:
kbd in a heading on this page

Code problem reported on this page.

Code looks a lot like the serif font in the notes now. Not sure how I feel about that. I'm wondering if we should add a background colour like github does.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 1, 2017

Codecov Report

Merging #4400 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4400   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files         254      254           
  Lines       22620    22620           
  Branches     1666     1666           
=======================================
  Hits        21428    21428           
  Misses        963      963           
  Partials      229      229

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ab1304...ece65bd. Read the comment docs.

codecov-io commented Sep 1, 2017

Codecov Report

Merging #4400 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4400   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files         254      254           
  Lines       22620    22620           
  Branches     1666     1666           
=======================================
  Hits        21428    21428           
  Misses        963      963           
  Partials      229      229

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ab1304...ece65bd. Read the comment docs.

@stephaniehobson stephaniehobson requested a review from schalkneethling Sep 1, 2017

@schalkneethling

This comment has been minimized.

Show comment
Hide comment
@schalkneethling

schalkneethling Sep 4, 2017

Collaborator

Ok, so I am not 100% on whether I tested the correct things here. Perhaps the title on the original offending page has been changed? Also, the page on production linked to contains a Kumascript error and so, I am not 100% sure that it is rendering correctly.

With that, my understanding is that the first problem is the way <kbd> is displayed when used in a header. Specifically a SlabZilla type heading. So, here is the way it would display currently:

screen shot 2017-09-04 at 13 35 56

And after the change it now displays as follows:

screen shot 2017-09-04 at 13 38 56

The next issue is the way inline <code> is displayed. Again before:

Linked code:

screen shot 2017-09-04 at 13 49 53

And when the code is not linked:

screen shot 2017-09-04 at 13 46 26

And here is unlinked code after the change applied in this PR:

screen shot 2017-09-04 at 13 50 18

I agree @stephaniehobson that it is hard to visually see the difference between plain copy and "code". I played around with your suggestion, re: adding a background colour, and came up with this:

screen shot 2017-09-04 at 13 52 26

The code for that would be:

font-family: monospace;
font-size: 16px;
background-color: lightgray;
padding: 2px 5px;

Let me know if I overlooked something, or did not test the appropriate things ;)

Collaborator

schalkneethling commented Sep 4, 2017

Ok, so I am not 100% on whether I tested the correct things here. Perhaps the title on the original offending page has been changed? Also, the page on production linked to contains a Kumascript error and so, I am not 100% sure that it is rendering correctly.

With that, my understanding is that the first problem is the way <kbd> is displayed when used in a header. Specifically a SlabZilla type heading. So, here is the way it would display currently:

screen shot 2017-09-04 at 13 35 56

And after the change it now displays as follows:

screen shot 2017-09-04 at 13 38 56

The next issue is the way inline <code> is displayed. Again before:

Linked code:

screen shot 2017-09-04 at 13 49 53

And when the code is not linked:

screen shot 2017-09-04 at 13 46 26

And here is unlinked code after the change applied in this PR:

screen shot 2017-09-04 at 13 50 18

I agree @stephaniehobson that it is hard to visually see the difference between plain copy and "code". I played around with your suggestion, re: adding a background colour, and came up with this:

screen shot 2017-09-04 at 13 52 26

The code for that would be:

font-family: monospace;
font-size: 16px;
background-color: lightgray;
padding: 2px 5px;

Let me know if I overlooked something, or did not test the appropriate things ;)

@stephaniehobson

This comment has been minimized.

Show comment
Hide comment
@stephaniehobson

stephaniehobson Sep 5, 2017

Contributor

Sounds like the right things were reviewed, thanks :)

Added a background colour for inline code to this PR now.

Contributor

stephaniehobson commented Sep 5, 2017

Sounds like the right things were reviewed, thanks :)

Added a background colour for inline code to this PR now.

@schalkneethling

r+ 🎫

@stephaniehobson stephaniehobson merged commit 72dba45 into master Sep 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jwhitlock jwhitlock deleted the 1370594-typography branch Sep 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment