Move CSS files to includes #1777

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
@penibelst
Member

penibelst commented Dec 5, 2013

Concatenating the 4 CSS files will speed up the docs. It also does a little minification with built-in liquid filters.

The 404.html (#1475) can now easily include inline CSS, according to Github Error Page Styleguide.

And I’ve updated normalize.css to 2.1.3.

@parkr

This comment has been minimized.

Show comment Hide comment
@parkr

parkr Dec 5, 2013

Owner

What's the speed-up for this?

Owner

parkr commented Dec 5, 2013

What's the speed-up for this?

@parkr

This comment has been minimized.

Show comment Hide comment
@parkr

parkr Dec 5, 2013

Owner

@cobyism have you seen this approach before?

Owner

parkr commented Dec 5, 2013

@cobyism have you seen this approach before?

@troyswanson

This comment has been minimized.

Show comment Hide comment
@troyswanson

troyswanson Dec 5, 2013

Member

The only real speed up it will have is on HTTP connections. 1 is better than 4.

Member

troyswanson commented Dec 5, 2013

The only real speed up it will have is on HTTP connections. 1 is better than 4.

+{% include css/pygments.css %}
+{% endcapture %}
+
+{{ screen | strip_newlines | remove: ' ' }}

This comment has been minimized.

Show comment Hide comment
@troyswanson

troyswanson Dec 5, 2013

Member

This is certainly interesting. Using Liquid and Jekyll to minify CSS. I don't see why it wouldn't work, though.

@troyswanson

troyswanson Dec 5, 2013

Member

This is certainly interesting. Using Liquid and Jekyll to minify CSS. I don't see why it wouldn't work, though.

@parkr

This comment has been minimized.

Show comment Hide comment
@parkr

parkr Dec 5, 2013

Owner

The only real speed up it will have is on HTTP connections. 1 is better than 4.

This depends upon latency. I'd prefer to see real numbers :)

Owner

parkr commented Dec 5, 2013

The only real speed up it will have is on HTTP connections. 1 is better than 4.

This depends upon latency. I'd prefer to see real numbers :)

@penibelst

This comment has been minimized.

Show comment Hide comment
@penibelst

penibelst Dec 6, 2013

Member

This depends upon latency. I'd prefer to see real numbers :)

Read Yahoo’s rules. Look at the real webpagetest results for jekyllrb.com.

Member

penibelst commented Dec 6, 2013

This depends upon latency. I'd prefer to see real numbers :)

Read Yahoo’s rules. Look at the real webpagetest results for jekyllrb.com.

@parkr

This comment has been minimized.

Show comment Hide comment
@parkr

parkr Dec 6, 2013

Owner

Boom, numbers! I'm 👍

Owner

parkr commented Dec 6, 2013

Boom, numbers! I'm 👍

@mattr-

This comment has been minimized.

Show comment Hide comment
@mattr-

mattr- Dec 7, 2013

Owner

It would be good if the change to the CSS was in one commit and the update to normalize was in another. This way we have better isolation in case something goes wrong with either of these changes.

Otherwise, 👍

Owner

mattr- commented Dec 7, 2013

It would be good if the change to the CSS was in one commit and the update to normalize was in another. This way we have better isolation in case something goes wrong with either of these changes.

Otherwise, 👍

@penibelst

This comment has been minimized.

Show comment Hide comment
@penibelst

penibelst Dec 7, 2013

Member

It would be good if the change to the CSS was in one commit and the update to normalize was in another.

Done.

Member

penibelst commented Dec 7, 2013

It would be good if the change to the CSS was in one commit and the update to normalize was in another.

Done.

@parkr

This comment has been minimized.

Show comment Hide comment
@parkr

parkr Dec 7, 2013

Owner

@penibelst We'd love it if you could force-push up a new history with two commits: one with the CSS movement & one with update to normalize 2.1.3 :) Sorry to be annoying, it'll make all the difference later on if we have a clean git history ❤️

Owner

parkr commented Dec 7, 2013

@penibelst We'd love it if you could force-push up a new history with two commits: one with the CSS movement & one with update to normalize 2.1.3 :) Sorry to be annoying, it'll make all the difference later on if we have a clean git history ❤️

@parkr parkr closed this in #1787 Dec 8, 2013

@penibelst penibelst deleted the penibelst:site-concatenate-css branch Dec 8, 2013

@penibelst

This comment has been minimized.

Show comment Hide comment
@penibelst

penibelst Dec 8, 2013

Member

@parkr Sorry for the dirty style, I will commit cleaner next time.

Member

penibelst commented Dec 8, 2013

@parkr Sorry for the dirty style, I will commit cleaner next time.

@parkr

This comment has been minimized.

Show comment Hide comment
@parkr

parkr Dec 8, 2013

Owner

No problemo :) Thanks for contributing in the first place!

Owner

parkr commented Dec 8, 2013

No problemo :) Thanks for contributing in the first place!

@penibelst

This comment has been minimized.

Show comment Hide comment
@penibelst

penibelst Dec 10, 2013

Member

Page tests for the home page jekyllrb.com

Member

penibelst commented Dec 10, 2013

Page tests for the home page jekyllrb.com

@mattr-

This comment has been minimized.

Show comment Hide comment
@mattr-

mattr- Dec 10, 2013

Owner

There’s virtually no difference. 

Owner

mattr- commented Dec 10, 2013

There’s virtually no difference. 

@mattr-

This comment has been minimized.

Show comment Hide comment
@mattr-

mattr- Dec 10, 2013

Owner

Not against the change at all, but perhaps I'm not seeing something I should be?

Owner

mattr- commented Dec 10, 2013

Not against the change at all, but perhaps I'm not seeing something I should be?

@cobyism

This comment has been minimized.

Show comment Hide comment
@cobyism

cobyism Dec 10, 2013

Member

First up, sorry I’m late to the party. Since I was asked, I just wanted to drop my opinion in here to reply even though this ended up being merged.

I’m all for making things faster, and this is definitely a clever trick (very handy to know, for future—thanks @penibelst!), however I’m unsure if this extra complexity is worth the trouble. Was speed/optimization really a problem in the site? I didn’t think there was a problem, but if there was, is shaving this amount of time (~100ms or so, according to those numbers?) off the initial page load time going to make a real difference? This is a static site anyway, so there’s virtually no server-side processing time, and the CSS files aren’t that big—so I guess I’m just left wondering why this became necessary. A wise man once said: “premature optimization is the root of all evil.”, and I’m inclined to agree.

The main downside I see is that now, if you see something that needs to be changed on the site, it’s going to be much harder to work out where exactly (source file & line number) the CSS needs to be changed, since the two now don’t match up in web inspector, and it’s not using source maps of any kind.

2013-12-10 at 2 11 pm

In many other environments (e.g. rails apps), the compression/concatenation and so forth is only enabled in production, so is disabled locally in development meaning you don’t have to worry about this issue. In this case, this is going to be the case in both production and development, with no easy way to switch it off. That’s not a major problem by any means, but it’s just another small nuisance to work around for anyone who wants to dig into this in the future.

Member

cobyism commented Dec 10, 2013

First up, sorry I’m late to the party. Since I was asked, I just wanted to drop my opinion in here to reply even though this ended up being merged.

I’m all for making things faster, and this is definitely a clever trick (very handy to know, for future—thanks @penibelst!), however I’m unsure if this extra complexity is worth the trouble. Was speed/optimization really a problem in the site? I didn’t think there was a problem, but if there was, is shaving this amount of time (~100ms or so, according to those numbers?) off the initial page load time going to make a real difference? This is a static site anyway, so there’s virtually no server-side processing time, and the CSS files aren’t that big—so I guess I’m just left wondering why this became necessary. A wise man once said: “premature optimization is the root of all evil.”, and I’m inclined to agree.

The main downside I see is that now, if you see something that needs to be changed on the site, it’s going to be much harder to work out where exactly (source file & line number) the CSS needs to be changed, since the two now don’t match up in web inspector, and it’s not using source maps of any kind.

2013-12-10 at 2 11 pm

In many other environments (e.g. rails apps), the compression/concatenation and so forth is only enabled in production, so is disabled locally in development meaning you don’t have to worry about this issue. In this case, this is going to be the case in both production and development, with no easy way to switch it off. That’s not a major problem by any means, but it’s just another small nuisance to work around for anyone who wants to dig into this in the future.

@penibelst penibelst referenced this pull request Dec 10, 2013

Merged

Unminify css #1803

@penibelst

This comment has been minimized.

Show comment Hide comment
@penibelst

penibelst Dec 10, 2013

Member

@mattr- Most browsers only issue 6 requests in parallel. Yahoo’s first performance rule is “Minimize HTTP Requests”. Jekyll’s website should follow best practices.

@cobyism I’ve unminified the CSS in #1803

If you want, I can undo everything before the discussion turns into bikeshedding.

Member

penibelst commented Dec 10, 2013

@mattr- Most browsers only issue 6 requests in parallel. Yahoo’s first performance rule is “Minimize HTTP Requests”. Jekyll’s website should follow best practices.

@cobyism I’ve unminified the CSS in #1803

If you want, I can undo everything before the discussion turns into bikeshedding.

@cobyism

This comment has been minimized.

Show comment Hide comment
@cobyism

cobyism Dec 10, 2013

Member

If you want, I can undo everything…

I’d just stick with it and see if it’s actually a problem. You’re contributing to this project much more than I am, so you (and anyone else contributing to the site regularly) will know if anything needs to be reverted sooner than I will.

Member

cobyism commented Dec 10, 2013

If you want, I can undo everything…

I’d just stick with it and see if it’s actually a problem. You’re contributing to this project much more than I am, so you (and anyone else contributing to the site regularly) will know if anything needs to be reverted sooner than I will.

@jekyllbot jekyllbot locked and limited conversation to collaborators Feb 27, 2017

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