-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
hugolib: improve menu generation for section pages with content #2978
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very useful, but needs a test or two.
hugolib/site.go
Outdated
URL: p.RelPermalink()} | ||
|
||
// menu with same id defined in config, let that one win | ||
if _, ok := flat[twoD{sectionPagesMenu, me.KeyName()}]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional can be pulled above the me above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I can squash all the commits if you wish, once we finish reviewing.
hugolib/site.go
Outdated
if sectionPagesMenu != "" { | ||
// Create menu entries for section pages with content | ||
for _, p := range pages { | ||
if p.Section() != "" && p.Kind == KindSection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check for p.Section()? Every page have a Kind -- and all section pages (with or without content) == KindSection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for p.Section() was already there before I touched it. You do not want to synthesize a section menu entry for the empty string, and that would happen if there are pages that do not belong to a section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, in this case you are right, on the second pass is when this test is supposed to be relevant.
} | ||
} | ||
|
||
// Create entries for remaining content-less section pages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by "content-less" section pages? The code below should be superfluous with the code above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest... I forgot most of it.
This piece of code was kind of ugly. It ran through all pages, collecting a list of all sections they belong to, and from this collection of section, synthesized menu entries.
What I remember, is that I thought I could do better, by running through all section pages, and assembling the menu entry via some method in Page, using the actual section page metadata. It didn't work, it turns out there was a reason for doing the old ugly thing. Probably content-less section pages are not yet there when the menu is assembled. Oh, by content-less I mean without a markdown file under content/.
Then I ended up refining the old method, by using the section page if it is there, and doing the old thing of synthesizing a menu entry for mentioned sections (not actual section pages) otherwise.
Maybe you, that are more familiar with the code can fill in the blanks. Otherwise I can try squeeze my brain a little more and see if anything else comes out.
When using the lazy blogger setting to automatically generate menu entries from section pages, we now recognize section pages that have content, and use the weight and linktitle configured in the frontmatter. This way, we can use the lazy blogger automatic generation, and influence menu order and translations, directly from the frontmatter. Fixes gohugoio#2974
4f0931f
to
0d78798
Compare
0d78798
to
7d62eb6
Compare
I'll try to add some tests tomorrow. Thanks for looking into this! |
I will finish this ins #3067 I'll need to do some general test cleanup in this department, and I might as well clean up after myself. Thanks for starting this and bringing this to my attention. |
Thanks! I'll try to do some testing of master before the upcoming release. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
When using the lazy blogger setting to automatically generate menu entries from section pages, we now recognize section pages that have content, and use the weight and linktitle configured in the frontmatter.
This way, we can use the lazy blogger automatic generation, and influence menu order and translations, directly from the frontmatter.
Fixes #2974