Skip to content

A small set of design changes#33

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

Closed
joarwilk wants to merge 5 commits into from
+16 −2
Closed

A small set of design changes #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
239ab9d
Improved legibility
joarwilk Aug 16, 2015
40d9a2a
Added base header styles
joarwilk Aug 16, 2015
b20c078
Added typekit fonts
joarwilk Aug 16, 2015
2beead1
Moved script tag
joarwilk Aug 16, 2015
f8b1a5e
Removed unnecessary style tag
joarwilk Aug 16, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions layouts/css/base.styl
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,9 +3,11 @@
// Base styles
body
box-sizing border-box
font 300 20px/1.5 'Source Sans Pro', Arial, sans-serif
color #000
font 400 15px/1.5 "source-sans-pro", sans-serif

@Fishrock123Fishrock123Aug 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please keep Arial, here as a fallback.

@fhembergerfhembergerAug 17, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why go below 16px as base font size?

@Fishrock123Fishrock123Aug 17, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm, was there anything wrong with 20px as base size? Medium uses about 20px for most of everything, makes it easy to read?

color #333333

@Fishrock123Fishrock123Aug 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel this may be taking too much contrast away for those who have issues regarding less contrasting text. See: nodejs/iojs.org#366

Perhaps #222?

@joarwilkjoarwilkAug 16, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed, will change.

margin 0
-webkit-font-smoothing antialiased

@fhembergerfhembergerAug 17, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should only be used for light text on dark backgrounds. Otherwise this may cause blurry text on Chrome/Windows. (So better have it on the main navigation an footer for example.)

text-rendering optimizeLegibility

@Fishrock123Fishrock123Aug 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmmm, I think some base styles were lost from io.js here.

I suggest:

html-webkit-font-smoothingantialiased-moz-osx-font-smoothinggrayscale-webkit-font-variant-ligaturesnonefont-variant-ligaturesnone

Please remove text-rendering optimizeLegibility for text that isn't fairly large. It is very slow and can negatively impact page load times.

@joarwilkjoarwilkAug 16, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah I didnt copy everything, was just a couple of base additions. I'll move the text rendering to headers only.


header,
main,
Expand All@@ -18,7 +20,16 @@ h3,
h4,
h5
font-weight normal
margin: 1em 0 10px 0

h1
font-size 30px
line-height 36px
margin 15px 0 11px

h2
font-size 25px
letter-spacing 1px

a,
a:link,
Expand Down
3 changes: 3 additions & 0 deletions layouts/partials/html-head.hbs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -18,4 +18,7 @@
<link rel="alternate" href="/{{site.locale}}/tsc-minutes.xml" title="Node.js TSC meeting minutes" type="application/rss+xml">
<link rel="stylesheet" href="/{{site.locale}}/styles.css" media="all">
<link rel="stylesheet" href="/static/css/prism-tomorrow.css" media="all">

<script src="//use.typekit.net/mse5tqx.js"></script>
<script>try{Typekit.load();}catch(e){}</script>

@Fishrock123Fishrock123Aug 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the exact need for this? How large is the file?

@joarwilkjoarwilkAug 16, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's for the "source-sans-pro" font

@Fishrock123Fishrock123Aug 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, wouldn't it be better to just include the font from link into css? That would make for less potential font resizing, and less javascript. :)

@joarwilkjoarwilkAug 16, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

9.8kb size, should be fine

@Fishrock123Fishrock123Aug 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right but this is two html requests compared to one. :)

@joarwilkjoarwilkAug 16, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I doubt we have the license to do that, I think typekit is necessary.

@Fishrock123Fishrock123Aug 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Naw, we should be able to, I've done it before. https://github.com/Fishrock123/Searchbeam/blob/master/src/less/fonts.less

@joarwilkjoarwilkAug 16, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah yeah you're right it's free to use, I'll embed it instead!

@fhembergerfhembergerAug 17, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It should only be embedded if the file size is < 2k, otherwise two requests are more performant than embedding.
You can also load an optimized version for the user's browser from Google Fonts:
https://www.google.com/fonts#QuickUsePlace:quickUse

For mobile use, it would be best to use the font-loading API and caching the fonts to localStorage to avoid the text being invisible until the web-font has finally loaded.

@mikealmikealAug 28, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We only use that font in one place on the front page which is why it's in a CSS include to google fonts on the front page. I don't know what the benefit of typekit is but if we do include it for this font we should only do so on the front page.

</head>