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

CSS tweaks #2127

Merged
merged 8 commits into from May 6, 2014

Conversation

Projects
None yet
6 participants
@XhmikosR
Contributor

XhmikosR commented Mar 12, 2014

Only thing left is to either group the selectors which use the svg base64 image or just remove it; there's already a background-color set so the background-image seems redundant to me.

Let me know about this and I can update the PR.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 13, 2014

If I remove the base64 svgs, style.css goes from 28.4KB to 23KB. Also, I don't think a browser that doesn't support gradient will support svg so this seems completely useless to me.

@mscharley

This comment has been minimized.

Contributor

mscharley commented Mar 13, 2014

For what it's worth, there are some, mostly mobile browsers from memory.
5KB, baked into a resource that everyone downloads anyway isn't really all
that much. How many gradients are there though that there's 5KB worth? A
single standard gradient image should be tiny, even base64'd.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 13, 2014

@mscharley: The problem isn't the size here, per se. The problem is that there's already a background-color fallback and a base64 svg for each gradient.

Which browser supports base64 svg and doesn't support gradients?

Fact is that as it is the svg fallback isjust bloat; I tried with IE7, which is the oldest browser I can try but I doubt anyone cares about it, and the gradients don't show, which is expected, but the fallback color isn't reached since there's a fallback SVG. You can see the SVG removal in the last patch in this PR.

@mscharley

This comment has been minimized.

Contributor

mscharley commented Mar 13, 2014

SVG works in IE9+ while you're stuck with filters for gradients right up to
11. Also some versions of Opera.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 13, 2014

Well, stuck or not, currently the gradients have everything: background-color, svgs, filters. Filters work fine for IE8+, as for Opera I doubt it's even 1% of the visitors.

@parkr

This comment has been minimized.

Member

parkr commented Mar 16, 2014

This would be for @cobyism's review as well. Or @troyswanson's. I know how to CSS, but these kind of judgement calls are still a bit beyond me.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 16, 2014

Ideally, I'd like some feedback as to which browser versions you care about for the website. With this PR, really old versions should be supported consistently, so it's easier to clean up the older vendor prefixes if you decide to officially not support them.

@cobyism

This comment has been minimized.

Member

cobyism commented Mar 17, 2014

👍 Thanks for the cleanup! Didn’t realise there was so much junk in there 😆

Which browser supports base64 svg and doesn't support gradients?

IE9 does not support gradients, but it does support SVG backgrounds, which is why these exist. If IE9 isn’t important enough as an audience to worry about gradients, then that’s totally fine and it should still look acceptable with just a flat BG, but that’s a decision for @parkr.

Personally, I don’t think the extra file size is causing any significant issues with the site, and it’s an unfortunate habit that as engineers we constantly feel compelled to (prematurely?) optimize at the expense of (albeit small) groups of users. This is a relatively small and low-traffic site in the grand scheme of things, and the fact that it’s a static site means it’s already going to be pretty fast since there’s no server-side processing time. If the extra nice touch of giving IE9 users gradients costs a KB or two and therefore maybe a few milliseconds of extra load time, then my vote would be that it’s worth keeping in.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 17, 2014

@cobyism: thanks for the feedback.

Here is what you have at the moment on master. Fallback background color for the gradients which isn't used at all, since the SVG image plays the fallback role. My suggestion was to just get rid of the SVG images, and use the simple background colors for those who are so unfortunate to still use an ancient browser. This seems like a fair trade to me...

Note, that all this isn't a premature optimization. It's in fact a big step towards cleaner CSS code for the future ;) Everyone tends to add new stuff, but only few people clean up the old stuff :D

Now, how about the vendor prefixes? With my PR all vendor prefixes are added, but should we even care for Chrome 3 for example? Or Firefox 3 for that matter. If you can give me a list of the supported browsers I can make sure the rules are followed throughout the file consistently.

@cobyism

This comment has been minimized.

Member

cobyism commented Mar 17, 2014

use the simple background colors for those who are so unfortunate to still use an ancient browser. This seems like a fair trade to me...

Yeah, like I said it depends how big of a priority it is for the project. I wouldn’t call IE9 ancient though—even on sites with very forward-thinking audiences it sees pretty heavy usage, often because of heavily locked down corporate IT environments. On GitHub for example, we typically see about 20% of our IE traffic coming from IE9. This is a reasonably tiny sliver of our overall usage though, and our CSS bundle is already pretty large, so we don’t bother with svg gradients. It all comes down to the requirements for the project in question.

Note, that all this isn't a premature optimization. It's in fact a big step towards cleaner CSS code for the future ;) Everyone tends to add new stuff, but only few people clean up the old stuff :D

Absolutely—I’m really grateful for and happy with the changes you’ve made, and I totally agree that cleaner code is going to make maintainability way easier for future contributors. My comment about premature optimization wasn’t a personal remark in any way, it was really in regard to weighing up the SVG gradients against CSS file size—as far as I see it, this isn’t a huge performance concern with the site at the moment, so it felt premature to use it to justify removal of gradients for IE9 users. To summarize, if the argument for removing gradients from the site for IE9 users is made on the basis that 1) the browser is ancient and 2) it’ll make noticeable difference to the site’s load time, then I would dispute both those assertions.

❤️

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 17, 2014

Well, I can't give you numbers since GitHub Pages does not allow us to test branch code, but there will be a difference, probably not huge, sure, but not negligible, in my opinion.

Anyway, I can put back the SVG fallbacks in order to get this merged. Anything else you'd like tweaked? How about the vendor prefixes?

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 17, 2014

On a side note, I strongly believe #2131 should be re-opened or at least, discussed more.

@parkr parkr closed this Mar 17, 2014

@parkr parkr reopened this Mar 17, 2014

@parkr parkr closed this Mar 17, 2014

@parkr parkr reopened this Mar 17, 2014

@parkr

This comment has been minimized.

Member

parkr commented Apr 2, 2014

Anything you still want me to include in here?

@parkr

This comment has been minimized.

Member

parkr commented Apr 2, 2014

Maybe we should hold off on this until we have SCSS support. 😃

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Apr 2, 2014

It's your call what you want to support :)

If you don't want to support like Firefox 3 we could drop many prefixes and so on. Otherwise, as things are now, all vendor prefixes have been added.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Apr 2, 2014

rebased

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Apr 23, 2014

@parkr: I think this should be merged at the moment. If there's SASS support, it would be the same to just remove the vendor prefixes and stuff.

@parkr

This comment has been minimized.

Member

parkr commented Apr 23, 2014

There are a bunch of things in there I don't think we want. This PR feels like it's too bloated.

@@ -9,7 +9,6 @@
html, body { height: 100%; }
body {
background-color: #FFF;

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

Why don't we want this background color?

This comment has been minimized.

@XhmikosR

XhmikosR Apr 23, 2014

Contributor

You are reviewing the PR in the wrong way; check the commits...

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

I think I'm allowed to review the PR how I choose, no? 😃

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

Additionally, the commits don't provide explanations/motivations.

This comment has been minimized.

@XhmikosR

XhmikosR Apr 23, 2014

Contributor

It's self-explanatory if you see the commit this change belongs to. It's duplicate.

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

I'm sorry to say that it's not self-explanatory for me! My apologies for my lack of knowledge in terms of the nuances of CSS here. Your commits explain the what, but not the why. I'm sorry I can't just know the why!

This comment has been minimized.

@XhmikosR

XhmikosR Apr 23, 2014

Contributor

While I see I could say some more stuff in my commits, the commits themselves are not teaching material. At least, I don't have the time to do that, hence the simplicity of my commit messages.

You can check https://github.com/CSSLint/csslint/wiki/

https://github.com/CSSLint/csslint/wiki/Disallow-duplicate-properties
https://github.com/CSSLint/csslint/wiki/Require-standard-property-with-vendor-prefix

Those should answer most of your questions.

-webkit-box-sizing: border-box;
-moz-box-sizing: border-box;
box-sizing: border-box;

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

Why does this matter?

This comment has been minimized.

@XhmikosR

XhmikosR Apr 23, 2014

Contributor

It's like CSS 101; a vendor prefix is supported before the non prefixed properties. And CSS properties follow the rule that the last one overwrites the previous ones.

color: #ddd;
background: #333;
background-color: #333;

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

Why do we need to be specific here? I hear it's always best to use background.

This comment has been minimized.

@XhmikosR

XhmikosR Apr 23, 2014

Contributor

You hear wrong then. background is a shorthand and sets all properties to its default unless you specify them.

@@ -31,7 +31,7 @@ body {
/* Sections */
body > header, body > section, body > footer {
header, section, footer {

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

This should be specific and not apply to all header, section, etc elements.

This comment has been minimized.

@XhmikosR

XhmikosR Apr 23, 2014

Contributor

Well, you do not have more than one header or footer, so it's specific already.

This comment has been minimized.

@kleinfreund

kleinfreund May 6, 2014

Contributor

There shouldn't be multiple header/section/footer elements as direct children of another element. In a document however, multiple occurence's of the elements in question are fine.

If the purpose of a CSS rule is to only select elements which are direct children (in this case of body), one should use classes on these elements. Otherwise a selector like body > section won't allow changes to the markup without adjusting the selector.

@@ -39,11 +39,11 @@ body > header, body > section, body > footer {
/* Header */
body > header h1, body > header nav {
header h1, header nav {

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

same

This comment has been minimized.

@XhmikosR

XhmikosR Apr 23, 2014

Contributor

same

display: inline-block;
}
body > header h1 span {
header h1 span {

This comment has been minimized.

@parkr

parkr Apr 23, 2014

Member

same

This comment has been minimized.

@XhmikosR

XhmikosR Apr 23, 2014

Contributor

same

@parkr

This comment has been minimized.

Member

parkr commented Apr 23, 2014

I don't really want all those vendored prefixes here :( They'll be deleted in like a week when 2.0 drops.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Apr 23, 2014

I have asked you already which browsers you want to support. I got no answer so I added support for all of them. I don't see how vendor prefixes will be gone, unless you mean using another tool.

parkr added a commit that referenced this pull request May 6, 2014

@parkr parkr merged commit a854a6a into jekyll:master May 6, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details

parkr added a commit that referenced this pull request May 6, 2014

@XhmikosR XhmikosR deleted the XhmikosR:css branch May 6, 2014

@jekyll jekyll 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.