Reworking site template to utilize Sass #2687

Merged
merged 12 commits into from Aug 6, 2014

Conversation

Projects
None yet
6 participants
@kleinfreund
Contributor

kleinfreund commented Aug 3, 2014

Alright @parkr, there it is. Got a bit bigger than I thought it would.

So folks, apologies for any mistakes I might have made. This is my first bigger pull request touching more than 2 lines of code at a time.

I'm open for questions/suggestions and won't be mad if this doesn't get merged. :)

Touched files:

in /lib/site_template:

.
├── _includes:
|   ├── header.html
|   ├── footer.html
|   └── head.html
├── _layouts:
|   ├── default.html
|   ├── page.html
|   └── post.html
├── _posts:
|   └── 0000-00-00-welcome-to-jekyll.markdown.erb
├── _sass:
|   ├── _base.scss
|   ├── _typography.scss
|   ├── _site-header.scss
|   ├── _site-footer.scss
|   ├── _page-content.scss
|   ├── _posts.scss
|   └── _syntax-highlighting.scss
├── css
|   └── main.scss
└── index.html

General

  • Reworded the Welcome to Jekyll post a bit. (plus correct apostrophes, yay: instead of ')
  • Small changes to breakpoints (750px break point is now 800px, matching .wrapper)

HTML/Markup changes

  • Making site-title a proper heading (h1)
  • Adjusting document outlines accordingly (post and page headings are now h2’s)
  • Changes to head.html just change order of the items so things are distinctable more easily.

CSS/Styles changes

  • Use box-sizing: border-box; globally
  • Using variables for fonts, colors and unified spacing
  • Code formatting
    • Removing .terminal styles because they weren’t used on the site
    • Moving code styles to general typography (they were nested inside .post before, but might as well appear outside of posts)
    • Removed prefixed property declarations for border-radius, because most browsers support the unprefixed version nowadays
    • Set overflow scrollbars for codeblocks only on horizontally, not vertically
  • Site header
    • Removed unnecessary background-color
    • Cleaning up some of the styles for the navigation
    • Removing unnecessary HTML attributes from the svg element (moving fill color to CSS)
  • Site footer
    • Using icon class for general icon styling
    • Removing unnecessary HTML attributes from the svg elements (moving fill color to CSS)
    • Simplifying the little grid in general
    • Removing widths with calc which isn’t needed with the alternate box model
@kleinfreund

This comment has been minimized.

Show comment
Hide comment
@kleinfreund

kleinfreund Aug 3, 2014

Contributor

Now that I'm seeing this monster here, I guess I should've grouped all this is single commits, huh? Let me google that…

Contributor

kleinfreund commented Aug 3, 2014

Now that I'm seeing this monster here, I guess I should've grouped all this is single commits, huh? Let me google that…

@barryclark

View changes

lib/site_template/_includes/footer.html
+ <a href="https://github.com/{{ site.github_username }}">
+ <span class="icon icon--github">
+ <svg viewBox="0 0 16 16">
+ <path d="M7.999,0.431c-4.285,0-7.76,3.474-7.76,7.761 c0,3.428,2.223,6.337,5.307,7.363c0.388,0.071,0.53-0.168,0.53-0.374c0-0.184-0.007-0.672-0.01-1.32 c-2.159,0.469-2.614-1.04-2.614-1.04c-0.353-0.896-0.862-1.135-0.862-1.135c-0.705-0.481,0.053-0.472,0.053-0.472 c0.779,0.055,1.189,0.8,1.189,0.8c0.692,1.186,1.816,0.843,2.258,0.645c0.071-0.502,0.271-0.843,0.493-1.037 C4.86,11.425,3.049,10.76,3.049,7.786c0-0.847,0.302-1.54,0.799-2.082C3.768,5.507,3.501,4.718,3.924,3.65 c0,0,0.652-0.209,2.134,0.796C6.677,4.273,7.34,4.187,8,4.184c0.659,0.003,1.323,0.089,1.943,0.261 c1.482-1.004,2.132-0.796,2.132-0.796c0.423,1.068,0.157,1.857,0.077,2.054c0.497,0.542,0.798,1.235,0.798,2.082 c0,2.981-1.814,3.637-3.543,3.829c0.279,0.24,0.527,0.713,0.527,1.437c0,1.037-0.01,1.874-0.01,2.129 c0,0.208,0.14,0.449,0.534,0.373c3.081-1.028,5.302-3.935,5.302-7.362C15.76,3.906,12.285,0.431,7.999,0.431z"/>

This comment has been minimized.

@barryclark

barryclark Aug 3, 2014

Contributor

No line-wrap 💖 💖 💖

Reads much nicer now.

@barryclark

barryclark Aug 3, 2014

Contributor

No line-wrap 💖 💖 💖

Reads much nicer now.

@barryclark

View changes

lib/site_template/_posts/0000-00-00-welcome-to-jekyll.markdown.erb
-
-You'll find this post in your `_posts` directory - edit this post and re-build (or run with the `-w` switch) to see your changes!
-To add new posts, simply add a file in the `_posts` directory that follows the convention: YYYY-MM-DD-name-of-post.ext.
+You’ll find this post in your `_posts` directory – edit it and re-build (or run with the `--watch` switch) to see your changes. To add new posts, simply add a file in the `_posts` directory that follows the convention: YYYY-MM-DD-name-of-post.ext.

This comment has been minimized.

@barryclark

barryclark Aug 3, 2014

Contributor

Thoughts on adding in a reminder to include front-matter here? Around half of the people I watch trying to create their first new post forget to include front matter and are confused as to why it doesn't work.

@barryclark

barryclark Aug 3, 2014

Contributor

Thoughts on adding in a reminder to include front-matter here? Around half of the people I watch trying to create their first new post forget to include front matter and are confused as to why it doesn't work.

This comment has been minimized.

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

I thought about adding a couple of things to this post, but also didn’t want to go two steps too far.

Something like this:

To add new posts, simply add a file in the `_posts` directory that follows the convention: YYYY-MM-DD-name-of-post.ext. Its title and layout is controlled by the Front Matter, don’t forget it:

{% highlight yaml %}
---
layout: post
title: "Welcome to Jekyll!"
---
You’ll find this post in your `_posts` directory…
{% endhighlight %}

When viewing the post, the user would also see the code behind the post (recursion!).

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

I thought about adding a couple of things to this post, but also didn’t want to go two steps too far.

Something like this:

To add new posts, simply add a file in the `_posts` directory that follows the convention: YYYY-MM-DD-name-of-post.ext. Its title and layout is controlled by the Front Matter, don’t forget it:

{% highlight yaml %}
---
layout: post
title: "Welcome to Jekyll!"
---
You’ll find this post in your `_posts` directory…
{% endhighlight %}

When viewing the post, the user would also see the code behind the post (recursion!).

This comment has been minimized.

@barryclark

barryclark Aug 3, 2014

Contributor

👍 I think that's really helpful.

@barryclark

barryclark Aug 3, 2014

Contributor

👍 I think that's really helpful.

This comment has been minimized.

@parkr

parkr Aug 3, 2014

Member

What about "To add new posts, simply add a file in the _posts directory that follows the convention YYYY-MM-DD-name-of-post.ext and includes the necessary front matter. Take a look at the source for this post to get an idea about how it works."

I don't think we need a code example in the source of the post itself – it should be simple and straight-forward enough to duplicate.

@parkr

parkr Aug 3, 2014

Member

What about "To add new posts, simply add a file in the _posts directory that follows the convention YYYY-MM-DD-name-of-post.ext and includes the necessary front matter. Take a look at the source for this post to get an idea about how it works."

I don't think we need a code example in the source of the post itself – it should be simple and straight-forward enough to duplicate.

@barryclark

View changes

lib/site_template/css/main.scss
-
-}
+---
+# The main Sass file needs front matter (the dashes are enough)

This comment has been minimized.

@barryclark

barryclark Aug 3, 2014

Contributor

I really like this comment. Super helpful. Possible prepend "Only" to read:

"Only the main Sass file needs front matter (the dashes are enough)"

@barryclark

barryclark Aug 3, 2014

Contributor

I really like this comment. Super helpful. Possible prepend "Only" to read:

"Only the main Sass file needs front matter (the dashes are enough)"

This comment has been minimized.

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

Agreed, adding this. :)

I also thought on adding a readme to the site_template dir or some explanatory stuff in the welcome post. Let’s see…

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

Agreed, adding this. :)

I also thought on adding a readme to the site_template dir or some explanatory stuff in the welcome post. Let’s see…

@barryclark

View changes

lib/site_template/_sass/_posts.scss
+ letter-spacing: -1px;
+ line-height: 1;
+
+ @media screen and (max-width: 800px) {

This comment has been minimized.

@barryclark

barryclark Aug 3, 2014

Contributor

What do you think about pulling the media query out to a mixin to avoid repetition?

Example: https://github.com/barryclark/jekyll-now/blob/master/_scss/_variables.scss#L22-L27

@barryclark

barryclark Aug 3, 2014

Contributor

What do you think about pulling the media query out to a mixin to avoid repetition?

Example: https://github.com/barryclark/jekyll-now/blob/master/_scss/_variables.scss#L22-L27

This comment has been minimized.

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

I thought about adding something like this, i.e. something inspired by the media query mixins Harry Roberts utilizes in his inuitcss (great stuff).

I'm about concerned about the added complexity, though. We want to keep it simple for new users. What do you think?

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

I thought about adding something like this, i.e. something inspired by the media query mixins Harry Roberts utilizes in his inuitcss (great stuff).

I'm about concerned about the added complexity, though. We want to keep it simple for new users. What do you think?

This comment has been minimized.

@barryclark

barryclark Aug 3, 2014

Contributor

The inuitcss mixin is really nice. That may be best to go with as it's very semantic and clear what's going on.

If you don't use a mixin it may be good to make 800px a variable so that it can be easily changed in one place.

@barryclark

barryclark Aug 3, 2014

Contributor

The inuitcss mixin is really nice. That may be best to go with as it's very semantic and clear what's going on.

If you don't use a mixin it may be good to make 800px a variable so that it can be easily changed in one place.

This comment has been minimized.

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

In between yours and Harrys’ is fine. Defining $palm: 600px; and $lap: 800px; and this?

@mixin media-query($device) {
    @media screen and (max-width: $device) {
        @content;
    }
}
@kleinfreund

kleinfreund Aug 3, 2014

Contributor

In between yours and Harrys’ is fine. Defining $palm: 600px; and $lap: 800px; and this?

@mixin media-query($device) {
    @media screen and (max-width: $device) {
        @content;
    }
}

This comment has been minimized.

@barryclark

barryclark Aug 3, 2014

Contributor

So then I'd use the following to include a media query:

@include media-query(palm) { 
  [styles here] 
}

Looks good to me! I wonder why he didn't $laptop instead of $lap. Seems like a worthwhile extra 3 characters.

@barryclark

barryclark Aug 3, 2014

Contributor

So then I'd use the following to include a media query:

@include media-query(palm) { 
  [styles here] 
}

Looks good to me! I wonder why he didn't $laptop instead of $lap. Seems like a worthwhile extra 3 characters.

This comment has been minimized.

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

Huh, don’t know. I guess it’s the same with .btn and .button, right? Not a big deal. :p

Thank you for your feedback, @barryclark. Helps me a lot.

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

Huh, don’t know. I guess it’s the same with .btn and .button, right? Not a big deal. :p

Thank you for your feedback, @barryclark. Helps me a lot.

This comment has been minimized.

@parkr

parkr Aug 3, 2014

Member

For this, I'd rather see @mixing on-tablets or something so users aren't confused about on what platforms the code applies. I think it'd read better:

@include on-tablets {
  .container { width: 80% }
}
@include on-mobiles {
  .container { width: 95% }
}
@parkr

parkr Aug 3, 2014

Member

For this, I'd rather see @mixing on-tablets or something so users aren't confused about on what platforms the code applies. I think it'd read better:

@include on-tablets {
  .container { width: 80% }
}
@include on-mobiles {
  .container { width: 95% }
}

This comment has been minimized.

@barryclark

barryclark Aug 3, 2014

Contributor

Glad it's helpful. I'm new to attempting OS contributions—it's fun to be a part of! 😄

@barryclark

barryclark Aug 3, 2014

Contributor

Glad it's helpful. I'm new to attempting OS contributions—it's fun to be a part of! 😄

@barryclark

This comment has been minimized.

Show comment
Hide comment
@barryclark

barryclark Aug 3, 2014

Contributor

Nice work @kleinfreund! Looking forward to hearing feedback from others too.

Contributor

barryclark commented Aug 3, 2014

Nice work @kleinfreund! Looking forward to hearing feedback from others too.

@parkr

View changes

lib/site_template/_sass/_syntax-highlighting.scss
+ background: #fff;
+ @extend %vertical-rhythm;
+
+ .c { color: #998; font-style: italic } /* Comment */

This comment has been minimized.

@parkr

parkr Aug 3, 2014

Member

Can you change all of these comments to // instead of /* */? They aren't needed in the CSS output, and they don't align with their actual output in the compiled file.

@parkr

parkr Aug 3, 2014

Member

Can you change all of these comments to // instead of /* */? They aren't needed in the CSS output, and they don't align with their actual output in the compiled file.

This comment has been minimized.

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

Oh, yes. Wow, that looks horrible. I don’t even understand what’s happening there. Thanks for the heads-up.

@kleinfreund

kleinfreund Aug 3, 2014

Contributor

Oh, yes. Wow, that looks horrible. I don’t even understand what’s happening there. Thanks for the heads-up.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 3, 2014

Member

Looking great!

Member

parkr commented Aug 3, 2014

Looking great!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 3, 2014

Member

/cc @jglovier for his thoughts if he has time to give them. 😄

Member

parkr commented Aug 3, 2014

/cc @jglovier for his thoughts if he has time to give them. 😄

@jglovier

This comment has been minimized.

Show comment
Hide comment
@jglovier

jglovier Aug 3, 2014

Member

😍 Overall 👍 here. Awesome to see this happen.

Making site-title a proper heading (h1)

👎 on this change. There are a couple schools of thought around marking up logos/site-titles, and I come from the school of thought that says your site title or logo is not a headline, it's basically meta information about your site. Think of a newspaper, for example. The newspaper name would be like your site-title/logo, and it's not a heading. The h1 of your newspaper is the actual headline of the featured article.

Some argue that you can/should use an h1 for your site-title/logo on the index, and switch it for post pages where the post headline becomes the h1. However I think that's bogus and that site wide consistency is preferred.

Anyway that's just my opinions/rationale for the existing markup. If you guys disagree and want to change it that's 🆒.

Use box-sizing: border-box; globally

Did you update all the padding to reflect this change as well? Otherwise it's going to bork styles everywhere.

I'm also generally 👎 on this change because while border-box is very well supported in modern browsers these days, and gaining a lot of adoption, it's not necessarily a best-practice and it's more of a preference. Not strongly opinionated here, just noting my perspective and why I didn't use it in the first place (although I typically do love using border-box on my own projects).

Using variables for fonts, colors and unified spacing

HUZZAH!! 🤘

Removing unnecessary HTML attributes from the svg element (moving fill color to CSS)

👎 on removing the fill attribute, because if stylesheets fail to load they provide a fallback so the SVG still renders properly.

Member

jglovier commented Aug 3, 2014

😍 Overall 👍 here. Awesome to see this happen.

Making site-title a proper heading (h1)

👎 on this change. There are a couple schools of thought around marking up logos/site-titles, and I come from the school of thought that says your site title or logo is not a headline, it's basically meta information about your site. Think of a newspaper, for example. The newspaper name would be like your site-title/logo, and it's not a heading. The h1 of your newspaper is the actual headline of the featured article.

Some argue that you can/should use an h1 for your site-title/logo on the index, and switch it for post pages where the post headline becomes the h1. However I think that's bogus and that site wide consistency is preferred.

Anyway that's just my opinions/rationale for the existing markup. If you guys disagree and want to change it that's 🆒.

Use box-sizing: border-box; globally

Did you update all the padding to reflect this change as well? Otherwise it's going to bork styles everywhere.

I'm also generally 👎 on this change because while border-box is very well supported in modern browsers these days, and gaining a lot of adoption, it's not necessarily a best-practice and it's more of a preference. Not strongly opinionated here, just noting my perspective and why I didn't use it in the first place (although I typically do love using border-box on my own projects).

Using variables for fonts, colors and unified spacing

HUZZAH!! 🤘

Removing unnecessary HTML attributes from the svg element (moving fill color to CSS)

👎 on removing the fill attribute, because if stylesheets fail to load they provide a fallback so the SVG still renders properly.

@kleinfreund

This comment has been minimized.

Show comment
Hide comment
@kleinfreund

kleinfreund Aug 4, 2014

Contributor

There are a couple schools of thought around marking up logos/site-titles, and I come from the school of thought that says your site title or logo is not a headline, it's basically meta information about your site.

I actually haven't heard of this yet, but I'm open for suggestions. What do others think about this? I can see your point here and give this some thoughts.

Edit: I looked around a bit and this answer on webmasters.stackexchange.com was what I was looking for. Good explanation and I agree with you, @jglovier. Removed the h1 from the site-title. Redoing the rest of the headings now.

I consider your second sentence about that bogus as well. Switching the headlines in such a case doesn't seem consistent to me.

Did you update all the padding to reflect this change as well? Otherwise it's going to bork styles everywhere.

Yes. I had 0.0.0.0:4000 opened the whole day and watched for possible unwanted changes to the layout, etc.

[…] it's not necessarily a best-practice and it's more of a preference.

Up to this point I never found a situation where instead of border-box I wanted the default (content-box) back. Being able to set relative width's and padding without adjusting the other value is a big deal, atleast for me.

Setting calc is perfectly fine here, but I think it's easier to use border-box.

[…] because if stylesheets fail to load they provide a fallback so the SVG still renders properly.

When no fill attribute and no fill property are present in the document, it defaults to black. For when the stylesheet doesn’t load, this seems fine for me, because the other colors aren’t present either in this case.


I’d like to here @parkr's opinion here. Thank you for your feedback. It is definitely helping me. :)

Contributor

kleinfreund commented Aug 4, 2014

There are a couple schools of thought around marking up logos/site-titles, and I come from the school of thought that says your site title or logo is not a headline, it's basically meta information about your site.

I actually haven't heard of this yet, but I'm open for suggestions. What do others think about this? I can see your point here and give this some thoughts.

Edit: I looked around a bit and this answer on webmasters.stackexchange.com was what I was looking for. Good explanation and I agree with you, @jglovier. Removed the h1 from the site-title. Redoing the rest of the headings now.

I consider your second sentence about that bogus as well. Switching the headlines in such a case doesn't seem consistent to me.

Did you update all the padding to reflect this change as well? Otherwise it's going to bork styles everywhere.

Yes. I had 0.0.0.0:4000 opened the whole day and watched for possible unwanted changes to the layout, etc.

[…] it's not necessarily a best-practice and it's more of a preference.

Up to this point I never found a situation where instead of border-box I wanted the default (content-box) back. Being able to set relative width's and padding without adjusting the other value is a big deal, atleast for me.

Setting calc is perfectly fine here, but I think it's easier to use border-box.

[…] because if stylesheets fail to load they provide a fallback so the SVG still renders properly.

When no fill attribute and no fill property are present in the document, it defaults to black. For when the stylesheet doesn’t load, this seems fine for me, because the other colors aren’t present either in this case.


I’d like to here @parkr's opinion here. Thank you for your feedback. It is definitely helping me. :)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 5, 2014

Member

Thanks for your detailed comment, @jglovier! Always great to know you'll reply to a call for help with detail, clarity, and honor. ⚡️

Disclaimer: I have always been more of a server guy myself, and don't have much knowledge of the latest trends in CSS. My personal site is an example of the simple things I ask for from CSS. 🍏

Being able to set relative width's and padding without adjusting the other value is a big deal, atleast for me

I don't really understand this. Does that mean I'll be up until dawn messing around with my new Jekyll site? I'm worried that new features adds overhead, even for someone like me that has been making websites since I was 12.

Setting calc is perfectly fine here, but I think it's easier to use border-box.

I have no idea how calc and border-box differ. Is one easier to use? Apologies for my naïveté.

[…] because if stylesheets fail to load they provide a fallback so the SVG still renders properly.

When no fill attribute and no fill property are present in the document, it defaults to black. For when the stylesheet doesn’t load, this seems fine for me, because the other colors aren’t present either in this case.

I think the fill should stay in the HTML as well. It makes it more interchangeable and deletable if I decide I don't want it. I think that is highly possible. Thus it becomes fully isolated in the HTML.

Member

parkr commented Aug 5, 2014

Thanks for your detailed comment, @jglovier! Always great to know you'll reply to a call for help with detail, clarity, and honor. ⚡️

Disclaimer: I have always been more of a server guy myself, and don't have much knowledge of the latest trends in CSS. My personal site is an example of the simple things I ask for from CSS. 🍏

Being able to set relative width's and padding without adjusting the other value is a big deal, atleast for me

I don't really understand this. Does that mean I'll be up until dawn messing around with my new Jekyll site? I'm worried that new features adds overhead, even for someone like me that has been making websites since I was 12.

Setting calc is perfectly fine here, but I think it's easier to use border-box.

I have no idea how calc and border-box differ. Is one easier to use? Apologies for my naïveté.

[…] because if stylesheets fail to load they provide a fallback so the SVG still renders properly.

When no fill attribute and no fill property are present in the document, it defaults to black. For when the stylesheet doesn’t load, this seems fine for me, because the other colors aren’t present either in this case.

I think the fill should stay in the HTML as well. It makes it more interchangeable and deletable if I decide I don't want it. I think that is highly possible. Thus it becomes fully isolated in the HTML.

@kleinfreund

This comment has been minimized.

Show comment
Hide comment
@kleinfreund

kleinfreund Aug 5, 2014

Contributor

@parkr calc and box-sizing: border-box; are different things, but tackled the same issue.

Our .wrapper limits our content to not be wider as max-width: 800px;, on less than 800 pixels, it would take 100% width of its parent (the viewport in this case). To still have some space for the content there, we set padding.

Now the default behavior for calculating the width of elements with the default box model is as follows:

Content width + border widths + paddings (roughly, there are edge cases for when content width is zero)

Problem? Yes, because we can’t set the total width of the element. We need to calculate. For when we set padding: 0 15px; to get a 800 pixel wide element, we would need to set max-width: 770px; (minus 2*15).

Both calc and box-sizing: border-box are a solution for this. With calc you simply can calculate the right max-width on the fly (even with relative units, Yeah!). And we’re done.

With border-box, we can set max-width: 800px; and padding: 0 15px; and the element would still be 800 pixels wide, but the content area would be smaller (which is what we want).

Box Sizing | CSS-Tricks – see the image at the top of the article. Visually clear.

I think the fill should stay in the HTML as well. It makes it more interchangeable and deletable if I decide I don't want it. I think that is highly possible. Thus it becomes fully isolated in the HTML.

I moved it to the CSS, because of seperation of markup and style. Having fill in the markup is essentially like having style="background-color: #efefef; on our body. Usually, changes to the styles should happen in the style sheets, not the HTML.

I suggest having fill as a fallback in the markup, but control it via CSS the fill property, no?

Contributor

kleinfreund commented Aug 5, 2014

@parkr calc and box-sizing: border-box; are different things, but tackled the same issue.

Our .wrapper limits our content to not be wider as max-width: 800px;, on less than 800 pixels, it would take 100% width of its parent (the viewport in this case). To still have some space for the content there, we set padding.

Now the default behavior for calculating the width of elements with the default box model is as follows:

Content width + border widths + paddings (roughly, there are edge cases for when content width is zero)

Problem? Yes, because we can’t set the total width of the element. We need to calculate. For when we set padding: 0 15px; to get a 800 pixel wide element, we would need to set max-width: 770px; (minus 2*15).

Both calc and box-sizing: border-box are a solution for this. With calc you simply can calculate the right max-width on the fly (even with relative units, Yeah!). And we’re done.

With border-box, we can set max-width: 800px; and padding: 0 15px; and the element would still be 800 pixels wide, but the content area would be smaller (which is what we want).

Box Sizing | CSS-Tricks – see the image at the top of the article. Visually clear.

I think the fill should stay in the HTML as well. It makes it more interchangeable and deletable if I decide I don't want it. I think that is highly possible. Thus it becomes fully isolated in the HTML.

I moved it to the CSS, because of seperation of markup and style. Having fill in the markup is essentially like having style="background-color: #efefef; on our body. Usually, changes to the styles should happen in the style sheets, not the HTML.

I suggest having fill as a fallback in the markup, but control it via CSS the fill property, no?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 5, 2014

Member

Thanks for that great explanation of calc and border-box. If they're the exact same thing, then let's go with whatever has better support amongst the browsers. Do you have a list of the browsers that support these attributes?

I moved it to the CSS, because of seperation of markup and style. Having fill in the markup is essentially like having style="background-color: #efefef; on our body. Usually, changes to the styles should happen in the style sheets, not the HTML.

I suggest having fill as a fallback in the markup, but control it via CSS the fill property, no?

I think it should only be in one place – having it in two places will bring just confusion to users trying to change this.

It feels like it's an integral part of the graphic itself, so I'd prefer to see it left in the <svg> document in the HTML.

Member

parkr commented Aug 5, 2014

Thanks for that great explanation of calc and border-box. If they're the exact same thing, then let's go with whatever has better support amongst the browsers. Do you have a list of the browsers that support these attributes?

I moved it to the CSS, because of seperation of markup and style. Having fill in the markup is essentially like having style="background-color: #efefef; on our body. Usually, changes to the styles should happen in the style sheets, not the HTML.

I suggest having fill as a fallback in the markup, but control it via CSS the fill property, no?

I think it should only be in one place – having it in two places will bring just confusion to users trying to change this.

It feels like it's an integral part of the graphic itself, so I'd prefer to see it left in the <svg> document in the HTML.

@jglovier

This comment has been minimized.

Show comment
Hide comment
@jglovier

jglovier Aug 5, 2014

Member

I'm worried that new features adds overhead, even for someone like me that has been making websites since I was 12.

Yeah, exactly why I opted not to use it in the first place. There would be cognitive overhead for users who aren't accustomed to the box-model because it affects how padding works.

As for browser support, CSS calc wins out over border-box, but that's really only a minor issue of why I would still suggest sticking with the traditional box model. This is a default theme. IMHO Anyone who wants to make minor hacks should be able to easily do so with the traditional model they are familiar with. Anyone who wants to get crazy with theming the Jekyll front-end will be comfortable with using things like border-box, so they burden should be on them to implement it if they want to.

Again, since it's a preference thing and not a clear best practice thing, I'd say let the default theme stick with the most familiar paradigms.

Here's caniuse.com browser support stats for CSS calc():

image

image

Here's caniuse.com browser support stats for border-box:

Note that primary green is full support, and off color green is partial support.

Member

jglovier commented Aug 5, 2014

I'm worried that new features adds overhead, even for someone like me that has been making websites since I was 12.

Yeah, exactly why I opted not to use it in the first place. There would be cognitive overhead for users who aren't accustomed to the box-model because it affects how padding works.

As for browser support, CSS calc wins out over border-box, but that's really only a minor issue of why I would still suggest sticking with the traditional box model. This is a default theme. IMHO Anyone who wants to make minor hacks should be able to easily do so with the traditional model they are familiar with. Anyone who wants to get crazy with theming the Jekyll front-end will be comfortable with using things like border-box, so they burden should be on them to implement it if they want to.

Again, since it's a preference thing and not a clear best practice thing, I'd say let the default theme stick with the most familiar paradigms.

Here's caniuse.com browser support stats for CSS calc():

image

image

Here's caniuse.com browser support stats for border-box:

Note that primary green is full support, and off color green is partial support.

@troyswanson

This comment has been minimized.

Show comment
Hide comment
@troyswanson

troyswanson Aug 5, 2014

Member

Chiming in on the box-sizing convo...

I might have missed something, but could we use a mixin to alleviate the need for calc and box-sizing? Send in the overall width and padding desired, and let Sass do the math.

Member

troyswanson commented Aug 5, 2014

Chiming in on the box-sizing convo...

I might have missed something, but could we use a mixin to alleviate the need for calc and box-sizing? Send in the overall width and padding desired, and let Sass do the math.

@kleinfreund

This comment has been minimized.

Show comment
Hide comment
@kleinfreund

kleinfreund Aug 6, 2014

Contributor

@troyswanson When using box-sizing: border-box; there isn’t even a need for math or calculations, because you hardly run in situations where a different behavior than that of border-box is needed. You set max-width and padding and it works. It figures the rest out for you.

I’m going to revert this now. It’s not a big deal, because it affected the behavior of the footer columns only. However I want to point out that in my opinion box-sizing: border-box; is one of these technologies that should be somewhat invoked upon users, because it is the more convenient and easier to use box model.

Contributor

kleinfreund commented Aug 6, 2014

@troyswanson When using box-sizing: border-box; there isn’t even a need for math or calculations, because you hardly run in situations where a different behavior than that of border-box is needed. You set max-width and padding and it works. It figures the rest out for you.

I’m going to revert this now. It’s not a big deal, because it affected the behavior of the footer columns only. However I want to point out that in my opinion box-sizing: border-box; is one of these technologies that should be somewhat invoked upon users, because it is the more convenient and easier to use box model.

@parkr

View changes

lib/site_template/_posts/0000-00-00-welcome-to-jekyll.markdown.erb
@@ -18,7 +18,7 @@ print_hi('Tom')
#=> prints 'Hi, Tom' to STDOUT.
{% endhighlight %}
-Check out the [Jekyll docs][jekyll] for more info on how to get the most out of Jekyll. File all bugs/feature requests at [Jekyll's GitHub repo][jekyll-gh].
+Check out the [Jekyll docs][jekyll] for more info on how to get the most out of Jekyll. File all bugs/feature requests at [Jekylls GitHub repo][jekyll-gh].

This comment has been minimized.

@parkr

parkr Aug 6, 2014

Member

Let's also plug the jekyll-help repo here:

If you have questions, you can ask them on [Jekyll's dedicated Help repository](https://github.com/jekyll/jekyll-help)
@parkr

parkr Aug 6, 2014

Member

Let's also plug the jekyll-help repo here:

If you have questions, you can ask them on [Jekyll's dedicated Help repository](https://github.com/jekyll/jekyll-help)
@parkr

View changes

lib/site_template/_posts/0000-00-00-welcome-to-jekyll.markdown.erb
-You'll find this post in your `_posts` directory - edit this post and re-build (or run with the `-w` switch) to see your changes!
-To add new posts, simply add a file in the `_posts` directory that follows the convention: YYYY-MM-DD-name-of-post.ext.
+To add new posts, simply add a file in the `_posts` directory that follows the convention YYYY-MM-DD-name-of-post.ext and includes the necessary front matter. Take a look at the source for this post to get an idea about how it works.

This comment has been minimized.

@parkr

parkr Aug 6, 2014

Member

Can you enclose the YYYY-MM-DD-name-of-post.ext in backticks please?

@parkr

parkr Aug 6, 2014

Member

Can you enclose the YYYY-MM-DD-name-of-post.ext in backticks please?

@parkr

View changes

lib/site_template/css/main.scss
+ "site-footer",
+ "page-content",
+ "posts",
+ "syntax-highlighting"

This comment has been minimized.

@parkr

parkr Aug 6, 2014

Member

For a starter site, this feels a little too complicated to me.

@parkr

parkr Aug 6, 2014

Member

For a starter site, this feels a little too complicated to me.

This comment has been minimized.

@barryclark

barryclark Aug 6, 2014

Contributor

I think combining these would work well:

  • site-header
  • site-footer
  • page-content
  • posts

Also combining:

  • base
  • typography
@barryclark

barryclark Aug 6, 2014

Contributor

I think combining these would work well:

  • site-header
  • site-footer
  • page-content
  • posts

Also combining:

  • base
  • typography
@kleinfreund

This comment has been minimized.

Show comment
Hide comment
@kleinfreund

kleinfreund Aug 6, 2014

Contributor

Combined the SCSS files (as @barryclark suggested) to _base.scss, _layout.scss and _syntax-highlighting.scss.

Also added the requested notes to the welcome post.

Contributor

kleinfreund commented Aug 6, 2014

Combined the SCSS files (as @barryclark suggested) to _base.scss, _layout.scss and _syntax-highlighting.scss.

Also added the requested notes to the welcome post.

@parkr

View changes

lib/site_template/_config.yml
@@ -9,6 +9,3 @@ baseurl: "" # the subpath of your site, e.g. /blog/
url: "http://yourdomain.com" # the base hostname & protocol for your site
twitter_username: jekyllrb
github_username: jekyll
-
-# Build settings
-markdown: kramdown

This comment has been minimized.

@parkr

parkr Aug 6, 2014

Member

Can you please put this back? 😄

@parkr

parkr Aug 6, 2014

Member

Can you please put this back? 😄

This comment has been minimized.

@kleinfreund

kleinfreund Aug 6, 2014

Contributor

Oh, right. Where is my head? Added it to my test site but not to the Jekyll clone.

@kleinfreund

kleinfreund Aug 6, 2014

Contributor

Oh, right. Where is my head? Added it to my test site but not to the Jekyll clone.

@parkr parkr merged commit f010515 into jekyll:master Aug 6, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

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

@jglovier

This comment has been minimized.

Show comment
Hide comment
@jglovier

jglovier Aug 6, 2014

Member

🤘 ⚡️ ❤️

Member

jglovier commented Aug 6, 2014

🤘 ⚡️ ❤️

@kleinfreund kleinfreund deleted the kleinfreund:sassify_site_template branch Aug 7, 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.