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

Html cleanup #2130

Merged
merged 9 commits into from Mar 26, 2014

Conversation

Projects
None yet
7 participants
@XhmikosR
Contributor

XhmikosR commented Mar 12, 2014

No description provided.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 12, 2014

I just noticed some of my changes are also in #1621...

@parkr parkr closed this Mar 17, 2014

@parkr parkr reopened this Mar 17, 2014

@parkr

This comment has been minimized.

Member

parkr commented Mar 23, 2014

@XhmikosR Would you mind rebasing for me?

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 23, 2014

Sure thing, will do when I'm back home and ping you.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 23, 2014

Done.

@parkr

This comment has been minimized.

Member

parkr commented Mar 23, 2014

Awesome, thank you.

So I've look this over and many of the changes seem, to me, quite trivial. I was wondering whether you could elaborate on your motivations for changing them. Would you oblige?

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 23, 2014

Sure, feel free to comment on the patches you want me to explain.

@@ -17,7 +17,7 @@
{% if site.google_analytics_id %}
<!-- Google Analytics (http://google.com/analytics) -->
<script type="text/javascript">
<script>

This comment has been minimized.

@parkr

parkr Mar 23, 2014

Member

What benefit does this add, or how is this 100% unnecessary?

This comment has been minimized.

@XhmikosR

XhmikosR Mar 24, 2014

Contributor

This type is useless since it's the default in HTML5.

@@ -157,8 +157,6 @@ body > footer img {
position: relative;
top: 8px;
margin-left: 5px;
width: 100px;
height: 30px;

This comment has been minimized.

@parkr

parkr Mar 23, 2014

Member

Why put these in the HTML? What do we gain from that?

This comment has been minimized.

@XhmikosR

XhmikosR Mar 24, 2014

Contributor

Well in theory, the HTML is parsed first, so we reduce the chances of reflowing. Personally I'm used to specifying the img dimensions in the HTML unless I make changes to the img in the CSS.

This comment has been minimized.

@gmile

gmile Mar 25, 2014

Contributor

I was curious about this too. According to Google, specifying dimentions directly on imaging is considered to be a best practice.

This comment has been minimized.

@XhmikosR

XhmikosR Mar 25, 2014

Contributor

Also, one shouldn't resize the images in the HTML; the images should be served scaled. But I left this for another time :D

@@ -1,7 +1,7 @@
{% assign items = include.items %}
{% for item in items %}
{% assign item_url = item | prepend:'/docs/' | append:'/' %}
{% assign item_url = item | prepend:"/docs/" | append:"/" %}

This comment has been minimized.

@parkr

parkr Mar 23, 2014

Member

Why are double quotes here better than single quotes?

This comment has been minimized.

@XhmikosR

XhmikosR Mar 24, 2014

Contributor

The quotes change is just a consistency one; both work the same of course. But as you can see only the cases I changed used single quotes.

@@ -7,7 +7,7 @@
<p>
Proudly hosted by
<a href="https://github.com">
<img src="{{ site.url }}/img/footer-logo.png" alt="GitHub • Social coding">
<img src="/img/footer-logo.png" width="100" height="30" alt="GitHub • Social coding">

This comment has been minimized.

@parkr

parkr Mar 23, 2014

Member

Why shouldn't we include the URL here?

This comment has been minimized.

@XhmikosR

XhmikosR Mar 24, 2014

Contributor

You don't even define that variable.

This comment has been minimized.

@Ivoz

Ivoz Mar 25, 2014

Contributor

Maybe this was meant to be site.baseurl? I make use of that all the time, otherwise jekyll sites with a custom baseurl won't render it properly

This comment has been minimized.

@parkr

parkr Mar 25, 2014

Member

I don't think so. We usually use site.url rather than site.baseurl because setting site.baseurl in our config would affect WEBrick.

@@ -31,65 +31,65 @@ class="flag">flags</code> (specified on the command-line) that control them.
</tr>
</thead>
<tbody>
<tr class='setting'>
<tr class="setting">

This comment has been minimized.

@parkr

parkr Mar 23, 2014

Member

Why are double quotes better here than single quotes? Why not just leave the quotes out altogether? All three variants are allowed.

This comment has been minimized.

@mscharley

mscharley Mar 23, 2014

Contributor

Removing quotes altogether is a bad idea, whatever the HTML spec says about it. Agreed that changing them is dubious at best though.

This comment has been minimized.

@XhmikosR

XhmikosR Mar 24, 2014

Contributor

Same answer as above. Nothing dubious.

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 23, 2014

I'm afk;  I'll reply to those tomorrow.

@parkr

This comment has been minimized.

Member

parkr commented Mar 23, 2014

Alright, thank you!

@parkr

This comment has been minimized.

Member

parkr commented Mar 25, 2014

Alright, thanks for handling my asinine questions, @XhmikosR. I'm 👍 on this guy if you are, @mattr-.

@parkr parkr added this to the 2.0 milestone Mar 25, 2014

parkr added a commit that referenced this pull request Mar 26, 2014

@parkr parkr merged commit 6706ce9 into jekyll:master Mar 26, 2014

1 check passed

default The Travis CI build passed
Details

parkr added a commit that referenced this pull request Mar 26, 2014

@XhmikosR XhmikosR deleted the XhmikosR:html-cleanup branch Mar 26, 2014

@parkr

This comment has been minimized.

Member

parkr commented Mar 26, 2014

Thanks @XhmikosR! Deployed to GHP 😃

@XhmikosR

This comment has been minimized.

Contributor

XhmikosR commented Mar 26, 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.