Skip to content
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

Provide exclude mechanism for minima #128

Closed
wants to merge 1 commit into from
Closed

Provide exclude mechanism for minima #128

wants to merge 1 commit into from

Conversation

dirkgomez
Copy link

Put this into your front matter to exclude pages:

exclude: true

@ashmaroli
Copy link
Member

To exclude a page from being processed, simply add published: false to the front matter

@pathawks
Copy link
Member

pathawks commented May 8, 2017

@ashmaroli It looks like this is about excluding a page from the default navigation, rather than preventing it from being published.

Copy link
Contributor

@benbalter benbalter left a comment

Choose a reason for hiding this comment

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

We'll also need to update the docs.

@@ -21,7 +21,7 @@
<div class="trigger">
{% for path in page_paths %}
{% assign my_page = site.pages | where: "path", path | first %}
{% if my_page.title %}
{% if my_page.title and !my_page.exclude != true %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend this be something more descriptive, like exclude_from_nav to prevent collisions with existing YAML.

Copy link
Author

Choose a reason for hiding this comment

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

Where in the docs would I put that? I can't find a section that seems a good fit to me (and I'm a Jekyll newbie, so a bit lost anyway).

Copy link

Choose a reason for hiding this comment

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

@dirkgomez Looking at the existing content, I would expect to see it as a new h3 in the Usage section

@ashmaroli
Copy link
Member

It looks like this is about excluding a page from the default navigation

@pathawks You're right. My bad..

@Strangehill
Copy link
Contributor

I find whitelisting pages with an include field rather than blacklisting with an exclude more useful more often.

@dirkgomez
Copy link
Author

Wouldn't that be a breaking change though?

@Strangehill
Copy link
Contributor

Oh, that's right. Good point.

@@ -21,7 +21,7 @@
<div class="trigger">
{% for path in page_paths %}
{% assign my_page = site.pages | where: "path", path | first %}
{% if my_page.title %}
{% if my_page.title and !my_page.exclude_from_nav != true %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be == true?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue is the unary not (!), which I understand is not supported in liquid. I believe this is sufficient:

if my_page.title and my_page.exclude_from_nav != true

Unfortunately, == false isn't available to us here because of a defect with truthiness in liquid/ruby with respect to booleans, nulls, empty string, uninitialized values, and so on...

Copy link

Choose a reason for hiding this comment

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

@dirkgomez I tested and confirmed @ashawley's logic to be correct. In the current state (8fb7b81) the boolean is working opposite from desired behavior.

Also, it is up to you if you care or not, but I noticed your first commit (41529ce) was done with a different e-mail address than the others, and that e-mail address is not associated with your GitHub account. You may wish to squash/rebase (then force-push) to bring them all under one address or to add your other e-mail address to your GitHub account so it is recognized as yours.

@@ -21,7 +21,7 @@
<div class="trigger">
{% for path in page_paths %}
{% assign my_page = site.pages | where: "path", path | first %}
{% if my_page.title %}
{% if my_page.title and my_page.exclude_from_nav == true %}
Copy link

Choose a reason for hiding this comment

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

Should be,

and my_page.exclude_from_nav != true

Thanks for sticking with this. I know from experience getting boolean logic right can be a pain :-)

Put this into your front matter to exclude pages from showing up in the
navigation:

exclude_from_nav: true
@dirkgomez
Copy link
Author

Amended the commits accordings - and thank you for the patience, still grappling with Jekyll.

@ashawley
Copy link
Contributor

ashawley commented Jun 7, 2017

Why isn't whitelisting with header_pages sufficient?

@dirkgomez
Copy link
Author

Looks very similar indeed, how do I use header_pages?

@ashawley
Copy link
Contributor

ashawley commented Jun 7, 2017

Looks like #52 documented it in the config but not the README

@dirkgomez
Copy link
Author

ashawley is right, I'm closing this pull request.

@dirkgomez dirkgomez closed this Jun 7, 2017
@ashawley
Copy link
Contributor

ashawley commented Jun 7, 2017

For the record, I wasn't opposed to this change. I was just wondering why the question wasn't raised about the overlap with header_pages. It takes a lot of humility to close a PR and then even further offer to make the documentation fix. Good on you, dirkgomez!

To be honest, I would still entertain this improvement. I predict it will come up again for scenarios where blacklisting may be easier to maintain than whitelisting (with header_pages).

@adamvoss
Copy link

adamvoss commented Jun 7, 2017

#134 continues from the discussion here by adding documentation for header_pages to **README.md.
(Commenting so the issues are linked on GitHub.)

@ashawley Like @dirkgomez I had need for something like this. I agree in not being opposed to this, but I am not sure there is enough reason to push for it now either. If you look at the behavior of minima when there are lots of nav links, I think from a practical standpoint the number of desired nav links will be kept low, favoring a whitelist approach. Plus, header_pages gives the added benefit of defining order.

The main advantage of the approach in this PR would be the resilience to file moves/renames.

@jekyll jekyll locked and limited conversation to collaborators Apr 30, 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.

None yet

8 participants