Skip to content

add header_pages config to link only specific files in header #52

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

Merged
merged 3 commits into from
Apr 7, 2017

Conversation

Crunch09
Copy link
Member

fixes #19

@Crunch09
Copy link
Member Author

cc: @benbalter

@@ -4,6 +4,17 @@

{% assign custom_url = site.url | append: site.baseurl %}
{% assign full_base_url = custom_url | default: site.github.url %}
{% if site.header_pages %}
{% assign header_pages = "" | split:"|" %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -4,6 +4,17 @@

{% assign custom_url = site.url | append: site.baseurl %}
{% assign full_base_url = custom_url | default: site.github.url %}
{% if site.header_pages %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be written as {% assign header_pages = site.header_pages | default: site.pages %}?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe, if we do that, we'll only need to iterate through the pages once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion! Yes we could do that,
but i think we should use something like this then:

{% assign default_paths = site.pages | map: "path" %}
{% assign header_pages = site.header_pages | default: default_paths %}

To make sure in both cases we're dealing with Strings and not in one case Strings, in the other one with Page objects.

In the remaining loop we could then convert these back to Pages one by one. What do you think?

@benbalter
Copy link
Contributor

Nice start! A few questions on the implementation to try to DRY things up a bit, but I really like it.

@Crunch09
Copy link
Member Author

@benbalter Thanks for the review! Improved it now to use only one loop

@scoskey
Copy link

scoskey commented Dec 17, 2016

Up vote.

@benbalter benbalter merged commit 55ff779 into jekyll:master Apr 7, 2017
@benbalter
Copy link
Contributor

Thanks @Crunch09! Sorry for the delay.

benbalter added a commit that referenced this pull request Apr 7, 2017
@Crunch09 Crunch09 deleted the header_pages_config branch April 8, 2017 13:40
@Crunch09
Copy link
Member Author

Crunch09 commented Apr 8, 2017

@benbalter No worries! Thank you for merging it.

domingohui pushed a commit to domingohui/minima that referenced this pull request Apr 14, 2017
cander added a commit to cander/etfarms that referenced this pull request Sep 28, 2017
Added a declaration of header_pages to control which pages show up in the
sidebar and their order.  Thats some undocumented voodoo I found in an issue
for the Minima theme jekyll/minima#52
Also a placeholder reading list and removed some obsolete stuff in the about
page.
@jekyll jekyll locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to specify Pages in Header
4 participants