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

Make navigation menus reusable #1507

Merged
merged 3 commits into from Sep 7, 2013

Conversation

Projects
None yet
5 participants
@doktorbro
Member

doktorbro commented Sep 6, 2013

This change introduces a reusable pattern to create dry menus.

The downside is a lot of whitespaces between block elements. After gzip the size increase is not noticeable.

Edit: The output HTML is unchanged to current solution.

@kelvinst

This comment has been minimized.

kelvinst commented Sep 6, 2013

That's pretty AWESOME!
❤️ 🎉

@parkr

This comment has been minimized.

Member

parkr commented Sep 7, 2013

This is pretty cool!

mattr- added a commit that referenced this pull request Sep 7, 2013

@mattr- mattr- merged commit 5a47609 into jekyll:master Sep 7, 2013

1 check passed

default The Travis CI build passed
Details
@mattr-

This comment has been minimized.

Member

mattr- commented Sep 7, 2013

🎉 Awesome stuff! Thanks!

mattr- added a commit that referenced this pull request Sep 7, 2013

@parkr

This comment has been minimized.

Member

parkr commented Sep 7, 2013

This really reduces readability though :(

@doktorbro doktorbro deleted the doktorbro:add-site-menu branch Sep 7, 2013

@doktorbro

This comment has been minimized.

Member

doktorbro commented Sep 8, 2013

This really reduces readability though :(

@parkr Do you mean readability of output HTML code? The output code is perfectly readable in Firebug or similar browser tools.

We can manually remove whitespace and other superfluous stuff from the input code:

<ul>{% assign items = include.items | split: ' ' %}{% for item in items %}{% assign item_url = item | prepend:'/docs/' | append:'/' %}{% if item_url == page.url %}{% assign current = true %}{% else %}{% assign current = false %}{% endif %}{% for p in site.pages %}{% if p.url == item_url %}<li{% if current %} class="current"{% endif %}><a href="{{ site.url }}{{ p.url }}">{{ p.title }}</a>{% endif %}{% endfor %}{% endfor %}</ul>

to get a minified version of the menu:

<ul><li><a href="/docs/frontmatter/">Front-matter</a><li><a href="/docs/posts/">Writing posts</a><li><a href="/docs/drafts/">Working with drafts</a><li><a href="/docs/pages/">Creating pages</a><li><a href="/docs/variables/">Variables</a><li><a href="/docs/migrations/">Blog migrations</a></ul>

And we can do all the variants between. The issue with whitespace is known by Liquid developers Shopify/liquid#226

The two general optimization questions are: which raw code must be as readable as possible and which code must be as small as possible? I think

  1. input—readable
  2. output—small

The input is already readable. As long you don’t minify the output HTML, you will find unwanted whitespace that hurts your website’s performance.

@parkr

This comment has been minimized.

Member

parkr commented Sep 8, 2013

I did not mean the outputted HTML. That looks fine (and few people ever read that near as I can tell). I'm talking about the convoluted include thing. It is just not as direct. You have to know way more about Jekyll to be able to understand anything that is happening.

@doktorbro

This comment has been minimized.

Member

doktorbro commented Sep 8, 2013

Should I add some comments?

{% comment %} to be able to understand anything that is happening. {% endcomment %}
@parkr

This comment has been minimized.

Member

parkr commented Sep 9, 2013

The best code doesn't need comments. ;)

@kelvinst

This comment has been minimized.

kelvinst commented Sep 9, 2013

Extremely agreed @parkr
Comments smells like 💩 for me (:

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