Skip to content

(PDS-380) Add Inconsolata as monospace font #176

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

Merged
merged 3 commits into from
Nov 21, 2019
Merged

Conversation

melcherry98
Copy link
Contributor

@melcherry98 melcherry98 commented Nov 20, 2019

Resolves PDS-380

  • Adds Code component

Screen Shot 2019-11-20 at 2 48 14 PM

@vine77
Copy link
Contributor

vine77 commented Nov 20, 2019

This PR proposes adding <Text code>. However, I wonder if instead we should add a <Code> component, keeping <Text> for Open Sans and <Heading> for Calibre. @nmuldavin, what do you think?

@vine77 vine77 changed the title adds Inconsolata as monospace font (PDS-380) Add Inconsolata as monospace font Nov 20, 2019
@nmuldavin
Copy link
Contributor

Yeah I vote <Code />. We should make the default html element for that new component a literal code block, though allow overriding with as

Copy link
Contributor

@vine77 vine77 left a comment

Choose a reason for hiding this comment

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

Thanks! Love that we have a Code component now. I think that'll be handy if we end up wanting to add props for a bordered code block or something. I left a couple comments about the docs.

I'm not sure why the x-height is taller when I view this in Styleguidist when compared to https://fonts.google.com/specimen/Inconsolata. Here are my screenshots:

Inconsolata on fonts.google.com:

inconsolata-google-fonts

Inconsolata example in Styleguidist:

inconsolata-styleguidist

My eye caught the fact that it looks a bit stretched out vertically, so maybe we can do a little more investigation to try to figure out why that's happening.

@melcherry98 melcherry98 requested a review from vine77 November 21, 2019 19:50
Copy link
Contributor

@vine77 vine77 left a comment

Choose a reason for hiding this comment

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

LGTM!

I even tried building Inconsolata from source, but got the same result.

@melcherry98 melcherry98 merged commit 27c92c0 into master Nov 21, 2019
@melcherry98 melcherry98 deleted the add-monospace-font branch November 21, 2019 21:55
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