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

Fix issue where "children" shortcode only shows top level. #252

Merged
merged 3 commits into from
Apr 16, 2019
Merged

Fix issue where "children" shortcode only shows top level. #252

merged 3 commits into from
Apr 16, 2019

Conversation

Chris-Greaves
Copy link
Contributor

As Explained in PR 194, there is a bug in the children shortcode where it only returns the top level.

This pull request is to fix this issue.

Unlike PR 194, this fix works on 0.26

@Chris-Greaves
Copy link
Contributor Author

image

Feel free to see it working on the deploy build:
https://deploy-preview-252--learn.netlify.com/en/shortcodes/children/

@worthlutz
Copy link

Limited testing shows it working except for when added to the "home" page, the initial _index.md in the root of the project. This page is reached by clicking the logo.

This may be because of differences of how hugo treats that initial page. I do not understand everything well enough to know...

What I am seeing is all pages listed as top level children even if depth="1". All appear as top level.

@Chris-Greaves
Copy link
Contributor Author

Sorry for the lack of in depth testing, when I tested it I created a couple of nested chapters, this worked fine, however when you use pages it completely breaks.

I will look at this over the next few days and update you when I have something.

@Chris-Greaves Chris-Greaves changed the title fix issue where "children" shortcode only shows top level. WIP: fix issue where "children" shortcode only shows top level. Mar 11, 2019
@Chris-Greaves
Copy link
Contributor Author

I have done some further testing and have pushed up a fix.

@worthlutz I have done more testing this time and have created a repo to demonstrate this here

this tests it using the following structure:

content
|   root-page-2.md
|   root-page-3.md
|   root-page.md
|   _index.md
|
+---children1
|   |   _index.md
|   |
|   \---1-1
|       |   _index.md
|       |
|       \---1-1-1
|           |   _index.md
|           |
|           \---1-1-1-1
|               |   _index.md
|               |
|               \---1-1-1-1-1
|                       _index.md
|
+---children2
|   |   _index.md
|   |
|   \---1-1
|       |   _index.md
|       |
|       \---1-1-1
|           |   _index.md
|           |
|           \---1-1-1-1
|               |   _index.md
|               |
|               \---1-1-1-1-1
|                       _index.md
|
+---children3
|   |   page2.md
|   |   _index.md
|   |
|   \---page3
|           page4.md
|           _index.md
|
\---children4
    |   page-1-a.md
    |   page-1-b.md
    |   page-1-c.md
    |   _index.md
    |
    \---level-1
        |   page-1-a.md
        |   page-1-b.md
        |   page-1-c.md
        |   _index.md
        |
        \---level-2
            |   page-1-a.md
            |   page-1-b.md
            |   page-1-c.md
            |   _index.md
            |
            \---level-3
                |   page-1-a.md
                |   page-1-b.md
                |   page-1-c.md
                |   _index.md
                |
                \---level-4
                        page-1-a.md
                        page-1-b.md
                        page-1-c.md
                        _index.md

@Chris-Greaves Chris-Greaves changed the title WIP: fix issue where "children" shortcode only shows top level. Fix issue where "children" shortcode only shows top level. Mar 12, 2019
@worthlutz
Copy link

@Chris-Greaves,
This looks good!

In the case where a sub-directory does not have an _index.md file, all the files in that sub-directory are shown as if they are up one directory. Is this the way things are supposed to work in hugo?

I seem to remember having read that the _index.md file is required but I cannot remember what is supposed to happen if it does not exist. Have not had time to look...

@Chris-Greaves
Copy link
Contributor Author

@worthlutz I believe that is the correct behaviour as Hugo classes the folder/_index.md as a Section, and every page comes under its parent section

https://gohugo.io/content-management/sections/#nested-sections

@worthlutz
Copy link

@Chris-Greaves I think that link must be what I read. Thanks for your work! I hope it gets merged soon.

@matalo33
Copy link
Contributor

matalo33 commented Apr 1, 2019

Hiya. Thanks very much for your contribution! I hope to include this fix in the next release.

Haven't had chance to fully review the change yet, however I did notice something odd when visiting the deploy preview.

On https://deploy-preview-252--learn.netlify.com/en/shortcodes/children/ the Children menu item has been relocated to the top of the section, when in fact the menu should remain alphabetically sorted. The menu is not resorted on the current live website (https://learn.netlify.com/en/shortcodes/children/)

Any thoughts on what causes this?

@Chris-Greaves
Copy link
Contributor Author

@matalo33 I don't have an idea why it would be doing that off the top of my head.

I will investigate when I get home and let you know.

@Chris-Greaves
Copy link
Contributor Author

Chris-Greaves commented Apr 2, 2019

@matalo33 So I did a git rebase and that doesn't seem to have made any difference.

I then ran it locally and it looks like the Live url

image

Having a look at some of the other Pull Requests, it looks like its something with the netlify build. Possibly uses a different version of hugo?

Looking at PR 259 you can see it also has it. https://deploy-preview-259--learn.netlify.com/en/shortcodes/

@Chris-Greaves
Copy link
Contributor Author

@matalo33 Any progress on this?

@matalo33
Copy link
Contributor

matalo33 commented Apr 16, 2019

Thanks, you are quite right this looks like an issue with the netlify build. Building locally with v0.54 doesn't break the menu ordering.

Thanks very much for your MR @Chris-Greaves

@matalo33 matalo33 merged commit 3d854fe into matcornic:master Apr 16, 2019
maxatome pushed a commit to maxatome/hugo-theme-learn that referenced this pull request Jan 23, 2024
maxatome pushed a commit to maxatome/hugo-theme-learn that referenced this pull request Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants