Use only the used Font Awesome icons. #5530

Merged
merged 1 commit into from Nov 29, 2016

Conversation

Projects
None yet
5 participants
@XhmikosR
Contributor

XhmikosR commented Nov 2, 2016

The font is generated with https://icomoon.io/app/.

This saves ~50KB.

A couple of questions:

  1. Is IE < 9 still supported? If not we could skip .eot.
  2. Do you want the json file from the IcoMoon app committed too?
+ src: url('../fonts/FontAwesome.eot?9h6hxj#iefix') format('embedded-opentype'),
+ url('../fonts/FontAwesome.ttf?9h6hxj') format('truetype'),
+ url('../fonts/FontAwesome.woff?9h6hxj') format('woff'),
+ url('../fonts/FontAwesome.svg?9h6hxj#FontAwesome') format('svg');

This comment has been minimized.

@pathawks

pathawks Nov 2, 2016

Member

WOFF is supported by 92% of browsers. What users are you targeting with all these new files?

@pathawks

pathawks Nov 2, 2016

Member

WOFF is supported by 92% of browsers. What users are you targeting with all these new files?

This comment has been minimized.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor

I suggest that you read about the bulletproof fontface.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor

I suggest that you read about the bulletproof fontface.

This comment has been minimized.

This comment has been minimized.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor

Yes. And you need to understand I don't make this by hand. This is how it was, this is how it was generated.

You want something else, make a new issue.

@XhmikosR

XhmikosR Nov 2, 2016

Contributor

Yes. And you need to understand I don't make this by hand. This is how it was, this is how it was generated.

You want something else, make a new issue.

This comment has been minimized.

@DirtyF

DirtyF Nov 2, 2016

Member

I thought SVG icons were the way to go now. :/

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 2, 2016

Contributor

Again guys that's a different issue. I simply made this pull request to
reduce the font size. Everything else is for a separate PR which I have no
time to submit myself.

On Nov 3, 2016 01:09, "Frank Taillandier" notifications@github.com wrote:

@DirtyF commented on this pull request.

In docs/_sass/_font-awesome.scss
#5530:

@font-face {
font-family: 'FontAwesome';

  • src: url('../fonts/fontawesome-webfont.eot?v=4.4.0');
  • src: url('../fonts/fontawesome-webfont.eot?#iefix&v=4.4.0') format('embedded-opentype'), url('../fonts/fontawesome-webfont.woff2?v=4.4.0') format('woff2'), url('../fonts/fontawesome-webfont.woff?v=4.4.0') format('woff'), url('../fonts/fontawesome-webfont.ttf?v=4.4.0') format('truetype'), url('../fonts/fontawesome-webfont.svg?v=4.4.0#fontawesomeregular') format('svg');
  • src: url('../fonts/FontAwesome.eot?9h6hxj');
  • src: url('../fonts/FontAwesome.eot?9h6hxj#iefix') format('embedded-opentype'),
  • url('../fonts/FontAwesome.ttf?9h6hxj') format('truetype'),
  • url('../fonts/FontAwesome.woff?9h6hxj') format('woff'),
  • url('../fonts/FontAwesome.svg?9h6hxj#FontAwesome') format('svg');

I thought SVG icons were the way to go
https://github.com/blog/2112-delivering-octicons-with-svg now.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#5530, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAVVtacsoiBZMe0R7ib9R306njmk83tWks5q6RgwgaJpZM4Knz1a
.

Contributor

XhmikosR commented Nov 2, 2016

Again guys that's a different issue. I simply made this pull request to
reduce the font size. Everything else is for a separate PR which I have no
time to submit myself.

On Nov 3, 2016 01:09, "Frank Taillandier" notifications@github.com wrote:

@DirtyF commented on this pull request.

In docs/_sass/_font-awesome.scss
#5530:

@font-face {
font-family: 'FontAwesome';

  • src: url('../fonts/fontawesome-webfont.eot?v=4.4.0');
  • src: url('../fonts/fontawesome-webfont.eot?#iefix&v=4.4.0') format('embedded-opentype'), url('../fonts/fontawesome-webfont.woff2?v=4.4.0') format('woff2'), url('../fonts/fontawesome-webfont.woff?v=4.4.0') format('woff'), url('../fonts/fontawesome-webfont.ttf?v=4.4.0') format('truetype'), url('../fonts/fontawesome-webfont.svg?v=4.4.0#fontawesomeregular') format('svg');
  • src: url('../fonts/FontAwesome.eot?9h6hxj');
  • src: url('../fonts/FontAwesome.eot?9h6hxj#iefix') format('embedded-opentype'),
  • url('../fonts/FontAwesome.ttf?9h6hxj') format('truetype'),
  • url('../fonts/FontAwesome.woff?9h6hxj') format('woff'),
  • url('../fonts/FontAwesome.svg?9h6hxj#FontAwesome') format('svg');

I thought SVG icons were the way to go
https://github.com/blog/2112-delivering-octicons-with-svg now.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#5530, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAVVtacsoiBZMe0R7ib9R306njmk83tWks5q6RgwgaJpZM4Knz1a
.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 2, 2016

Member

@XhmikosR I hear you, thanks for pointing that this could be improved, wasn''t aware of this before your PR.

Member

DirtyF commented Nov 2, 2016

@XhmikosR I hear you, thanks for pointing that this could be improved, wasn''t aware of this before your PR.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 2, 2016

Member

@XhmikosR Reducing file size is certainly a good thing, and I am all 👍 on that.

The fact remains, however, that this PR is adding three additional font formats to the CSS, and that is something that cannot be done without good reason. I'm not saying it mustn't be done; I’' only saying that the motivation is unclear to me.

Member

pathawks commented Nov 2, 2016

@XhmikosR Reducing file size is certainly a good thing, and I am all 👍 on that.

The fact remains, however, that this PR is adding three additional font formats to the CSS, and that is something that cannot be done without good reason. I'm not saying it mustn't be done; I’' only saying that the motivation is unclear to me.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 3, 2016

Member

@pathawks yeah, I find it too bad that better compressed woff2 has gone 😕

Member

DirtyF commented Nov 3, 2016

@pathawks yeah, I find it too bad that better compressed woff2 has gone 😕

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 3, 2016

Contributor

@pathawks: where on earth do you see this PR is adding 3 new formats? Please don't spread BS.

Contributor

XhmikosR commented Nov 3, 2016

@pathawks: where on earth do you see this PR is adding 3 new formats? Please don't spread BS.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 3, 2016

Contributor

And seriously, I have no interest on discussing this except for the points I make on my OP.

@parkr: please have a look and cmerge or close.

Contributor

XhmikosR commented Nov 3, 2016

And seriously, I have no interest on discussing this except for the points I make on my OP.

@parkr: please have a look and cmerge or close.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 3, 2016

Member

@XhmikosR Previously _font-awesome.scss had only EOT. This PR adds TTF, WOFF, and SVG. I am only discussing the changes you have proposed in this PR. If this change was a mistake, it should be fixed. If I am misunderstanding this change, I would like an explanation.

I am not trying to be difficult, only trying to understand the implications of this PR.

Member

pathawks commented Nov 3, 2016

@XhmikosR Previously _font-awesome.scss had only EOT. This PR adds TTF, WOFF, and SVG. I am only discussing the changes you have proposed in this PR. If this change was a mistake, it should be fixed. If I am misunderstanding this change, I would like an explanation.

I am not trying to be difficult, only trying to understand the implications of this PR.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 3, 2016

Contributor

No it does not have only eot. Scroll to the right.

Contributor

XhmikosR commented Nov 3, 2016

No it does not have only eot. Scroll to the right.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 3, 2016

Member

That’s what I was missing! So sorry for the confusion.

Member

pathawks commented Nov 3, 2016

That’s what I was missing! So sorry for the confusion.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 3, 2016

Contributor

Yeah, that is my whole point of asking if IE < 9 is supported ;) If not, I could skip .eot.

Contributor

XhmikosR commented Nov 3, 2016

Yeah, that is my whole point of asking if IE < 9 is supported ;) If not, I could skip .eot.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 3, 2016

Member

Use only the used Font Awesome icons

If we want to use another icon, we have to regenerate this, right? How do you feel about the trade-off between saving KB and adding development time? Not sure how frequently we use new icons.

Is IE < 9 still supported? If not we could skip .eot.

I don't believe IE < 9 is supported, no. If it doesn't add any bandwidth to the request for IE > 9, Safari, Firefox, and Chrome, then I'd be fine leaving it in as it sounds like we had .eot before this PR.

Do you want the json file from the IcoMoon app committed too?

What is this JSON file? Can you paste in a gist and paste the link in a reply here? How does IcoMoon play in? Maybe we need documentation in the CSS about how to updated FontAwesome icons – I have no idea what IcoMoon is and how to use it.

Member

parkr commented Nov 3, 2016

Use only the used Font Awesome icons

If we want to use another icon, we have to regenerate this, right? How do you feel about the trade-off between saving KB and adding development time? Not sure how frequently we use new icons.

Is IE < 9 still supported? If not we could skip .eot.

I don't believe IE < 9 is supported, no. If it doesn't add any bandwidth to the request for IE > 9, Safari, Firefox, and Chrome, then I'd be fine leaving it in as it sounds like we had .eot before this PR.

Do you want the json file from the IcoMoon app committed too?

What is this JSON file? Can you paste in a gist and paste the link in a reply here? How does IcoMoon play in? Maybe we need documentation in the CSS about how to updated FontAwesome icons – I have no idea what IcoMoon is and how to use it.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 3, 2016

Contributor

The json file is what you feed to the app to generate the font without
doing everything from scratch.

Generally this is a fine solution IMO. JUST LOAD THE JSON file, choose the
icons you want and you are done.

On Nov 3, 2016 18:25, "Parker Moore" notifications@github.com wrote:

Use only the used Font Awesome icons

If we want to use another icon, we have to regenerate this, right? How do
you feel about the trade-off between saving KB and adding development time?
Not sure how frequently we even need to add

Is IE < 9 still supported? If not we could skip .eot.

I don't believe IE < 9 is supported, no. If it doesn't add any bandwidth
to the request for IE > 9, Safari, Firefox, and Chrome, then I'd be fine
leaving it in as it sounds like we had .eot before this PR.

Do you want the json file from the IcoMoon app committed too?

What is this JSON file? Can you paste in a gist and paste the link in a
reply here? How does IcoMoon play in? Maybe we need documentation in the
CSS about how to updated FontAwesome icons – I have no idea what IcoMoon is
and how to use it.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5530 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAVVtQU3HWwYjRMHxUoNzS6BNAYy7VfEks5q6gr0gaJpZM4Knz1a
.

Contributor

XhmikosR commented Nov 3, 2016

The json file is what you feed to the app to generate the font without
doing everything from scratch.

Generally this is a fine solution IMO. JUST LOAD THE JSON file, choose the
icons you want and you are done.

On Nov 3, 2016 18:25, "Parker Moore" notifications@github.com wrote:

Use only the used Font Awesome icons

If we want to use another icon, we have to regenerate this, right? How do
you feel about the trade-off between saving KB and adding development time?
Not sure how frequently we even need to add

Is IE < 9 still supported? If not we could skip .eot.

I don't believe IE < 9 is supported, no. If it doesn't add any bandwidth
to the request for IE > 9, Safari, Firefox, and Chrome, then I'd be fine
leaving it in as it sounds like we had .eot before this PR.

Do you want the json file from the IcoMoon app committed too?

What is this JSON file? Can you paste in a gist and paste the link in a
reply here? How does IcoMoon play in? Maybe we need documentation in the
CSS about how to updated FontAwesome icons – I have no idea what IcoMoon is
and how to use it.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5530 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAVVtQU3HWwYjRMHxUoNzS6BNAYy7VfEks5q6gr0gaJpZM4Knz1a
.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 9, 2016

Member

Saving 50kb is not the right measurement here IMHO.

Since this PR is intended to address performance issues, I just ran a test on webpagetest of the current loading on mobile. Chrome loads woff2 format by default as all other modern browsers do except IE.

@XhmikosR have you ran some tests that shows that rendering is faster with this PR?

Member

DirtyF commented Nov 9, 2016

Saving 50kb is not the right measurement here IMHO.

Since this PR is intended to address performance issues, I just ran a test on webpagetest of the current loading on mobile. Chrome loads woff2 format by default as all other modern browsers do except IE.

@XhmikosR have you ran some tests that shows that rendering is faster with this PR?

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 9, 2016

Contributor

Of course it's faster. And in the worst case it simply saves 59 kb of crap
that are not used so it saves bandwidth.

On Nov 9, 2016 02:53, "Frank Taillandier" notifications@github.com wrote:

Saving 50kb is not the right measurement here IMHO.

Since this PR is intended to address performance issues, I just ran a test
on webpagetest
https://www.webpagetest.org/result/161109_1S_186/2/details/#waterfall_view_step1
of the current loading on mobile. Chrome load woff2 format by default as
all other modern browsers do except IE.

@XhmikosR https://github.com/XhmikosR have you ran some tests that
shows that rendering is faster with this PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5530 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAVVtQnLYsOXDXGTdzkrQz_7fQAT5rk-ks5q8RlpgaJpZM4Knz1a
.

Contributor

XhmikosR commented Nov 9, 2016

Of course it's faster. And in the worst case it simply saves 59 kb of crap
that are not used so it saves bandwidth.

On Nov 9, 2016 02:53, "Frank Taillandier" notifications@github.com wrote:

Saving 50kb is not the right measurement here IMHO.

Since this PR is intended to address performance issues, I just ran a test
on webpagetest
https://www.webpagetest.org/result/161109_1S_186/2/details/#waterfall_view_step1
of the current loading on mobile. Chrome load woff2 format by default as
all other modern browsers do except IE.

@XhmikosR https://github.com/XhmikosR have you ran some tests that
shows that rendering is faster with this PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5530 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAVVtQnLYsOXDXGTdzkrQz_7fQAT5rk-ks5q8RlpgaJpZM4Knz1a
.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 9, 2016

Member

My point here is that it seems to be more important to keep woff2 format for a majority of browsers, and that this will assure browsers download the lighter format.

Member

DirtyF commented Nov 9, 2016

My point here is that it seems to be more important to keep woff2 format for a majority of browsers, and that this will assure browsers download the lighter format.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 9, 2016

Contributor

Your point is moot. It's not lighter.

On Nov 9, 2016 02:56, "Frank Taillandier" notifications@github.com wrote:

My point here it's that it seems to it's more important to keep woff2
format for a majority of browsers, and that this will assure browsers
download the lighter format.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5530 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAVVtTJvXDIyKAXFTUJiWtlEMs_w5XaMks5q8RpGgaJpZM4Knz1a
.

Contributor

XhmikosR commented Nov 9, 2016

Your point is moot. It's not lighter.

On Nov 9, 2016 02:56, "Frank Taillandier" notifications@github.com wrote:

My point here it's that it seems to it's more important to keep woff2
format for a majority of browsers, and that this will assure browsers
download the lighter format.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5530 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAVVtTJvXDIyKAXFTUJiWtlEMs_w5XaMks5q8RpGgaJpZM4Knz1a
.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 9, 2016

Member

Thanks for the PR @XhmikosR. I'd be happy to accept it if you are willing to write a quick README or comment in the generated file which explains how to modify the file. If there is configuration required, then let's commit that here (or put it in the README) as well so that isn't lost for future folks.

Member

parkr commented Nov 9, 2016

Thanks for the PR @XhmikosR. I'd be happy to accept it if you are willing to write a quick README or comment in the generated file which explains how to modify the file. If there is configuration required, then let's commit that here (or put it in the README) as well so that isn't lost for future folks.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 11, 2016

Contributor

@parkr: where do you want me to place the info? I can upload the json file in the fonts folder or do you have something else in mind?

Contributor

XhmikosR commented Nov 11, 2016

@parkr: where do you want me to place the info? I can upload the json file in the fonts folder or do you have something else in mind?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 29, 2016

Member

@parkr: where do you want me to place the info? I can upload the json file in the fonts folder or do you have something else in mind?

JSON file in the fonts folder would work, or make a README with instructions. I don't even know how to update this! A README with instructions would be lovely.

Member

parkr commented Nov 29, 2016

@parkr: where do you want me to place the info? I can upload the json file in the fonts folder or do you have something else in mind?

JSON file in the fonts folder would work, or make a README with instructions. I don't even know how to update this! A README with instructions would be lovely.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 29, 2016

Contributor

@parkr: can you check if now everything is as you want?

Contributor

XhmikosR commented Nov 29, 2016

@parkr: can you check if now everything is as you want?

Use only the used Font Awesome icons.
The font is generated with https://icomoon.io/app/.

This saves ~50KB.
@parkr

parkr approved these changes Nov 29, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 29, 2016

Member

AppVeyor failed because the fmt test timed out. No clue why that would happen as I don't know the nuances of bash on AppVeyor Windows.

Member

parkr commented Nov 29, 2016

AppVeyor failed because the fmt test timed out. No clue why that would happen as I don't know the nuances of bash on AppVeyor Windows.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 29, 2016

Member

@jekyllbot: merge +site

Member

parkr commented Nov 29, 2016

@jekyllbot: merge +site

@jekyllbot jekyllbot merged commit 293b6f1 into jekyll:master Nov 29, 2016

1 of 2 checks passed

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

jekyllbot added a commit that referenced this pull request Nov 29, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 29, 2016

Member

Thanks for sticking with this, @XhmikosR! Hooray for saving bandwidth and reducing page size. 👏

Member

parkr commented Nov 29, 2016

Thanks for sticking with this, @XhmikosR! Hooray for saving bandwidth and reducing page size. 👏

@XhmikosR XhmikosR referenced this pull request Dec 1, 2016

Merged

Update _font-awesome.scss #5614

parkr added a commit that referenced this pull request Dec 16, 2016

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