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 alternative way to determining whether a page ends up in top navigation #340

Closed
wants to merge 1 commit into from

Conversation

wssbck
Copy link

@wssbck wssbck commented Jul 5, 2017

To determine whether a page is being shown in top navigation, do not rely on certain tags present in pages. Instead, allow for an explicit setting in page properties.

@ashawley
Copy link
Collaborator

ashawley commented Jul 5, 2017

Instead of introducing a new config variable in the front matter, pixyll should try to be more consistent with other jekyll themes and use the header_pages variable in the site config. Here's the config of the default theme https:/jekyll/minima/blob/master/_config.yml that is used with jekyll new site command. I'm not sure if it's used in others, though.

I've made this change for pixyll in #334, which is a PR for making the theme in to a gem. I'm happy to submit that change as a separate PR from the gem one. Since this is a breaking change, I'd say that it's worth making the change to be consistent with other themes, than not.

@wssbck
Copy link
Author

wssbck commented Jul 5, 2017

@ashawley Didn't know about that, thanks for suggesting. If you have nothing against it, I could replicate your change on this PR. However if you would prefer to open your own, please do, of course

@ashawley
Copy link
Collaborator

ashawley commented Jul 5, 2017

Sure, I'll work on pulling that change out to a new PR

@johno
Copy link
Owner

johno commented Jul 6, 2017

Thanks for opening up this pr, we've incorporated your proposed in #342 so I'll go ahead and close this now 💟

@johno johno closed this Jul 6, 2017
@wssbck wssbck deleted the nav_pages branch July 8, 2017 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants