getzola / book

Gitbook theme for Zola
MIT License
57 stars 19 forks source link

Fix broken previous and next links #1

Closed codesections closed 6 years ago

codesections commented 6 years ago

The existing previous and next links seem to point in the wrong directions. They are next and previous in a blog order (i.e., chronology), but the sidebar menu presents the links in descending order. This could also be fixed by changing the order in the sidebar menu, but this fix seems more straightforward.

To see a demo of the existing, broken functionality, see this demo site. Pressing the previous arrow from section 4.3 should take you to 4.2 but actually takes you to 4.4. The next arrow is similarly broken.

codesections commented 6 years ago

I updated the pull request with a commit that gets the desired behavior while preserving the semantics of page.next.permalink and page.previous.permalink. It does this by reversing the order of the loop, which has an added advantage: it means that the "order" semantics become more intutative. That is, a page with order = 1 would be presented as the first within its section, not the last.

In the process of making this change, I discovered that the loop over the index.subsections array presents pages in random order if no wieght is set. I opened an issue about this (Keats/gutenberg#335) but, in the meantime, have added a note in the README instructing the user to set both weight and order.

I also added a note to the README about the redirect issue from #2, which should close that issue. I did that here instead of in the seperate pull request to avoid a merge conflict.

Keats commented 6 years ago

It really looks I messed up when doing the initial theme/templates, I'll have a look on my own later

codesections commented 6 years ago

Sounds good. It looked ok in my testing, so I'm sorry to hear there were issues. If you'd like, I'd be happy to dig more into this—just let me know what the issue is if you'd like me to keep at it.

Keats commented 6 years ago

I switched to using weight for pages and I'm thinking now if pages with weight are not populated correctly. Adding this test to Gutenberg:


    #[test]
    fn can_populate_previous_and_next_pages_with_weight() {
        let input = vec![
            create_page_with_weight(1, "a.md"),
            create_page_with_weight(2, "b.md"),
            create_page_with_weight(3, "a.md"),
        ];
        let pages = populate_previous_and_next_pages(&input);

        assert!(pages[0].clone().previous.is_none());
        assert!(pages[0].clone().next.is_some());
        assert_eq!(pages[0].clone().next.unwrap().meta.weight.unwrap(), 2);

        assert!(pages[1].clone().next.is_some());
        assert!(pages[1].clone().previous.is_some());
        assert_eq!(pages[1].clone().previous.unwrap().meta.weight.unwrap(), 1);
        assert_eq!(pages[1].clone().next.unwrap().meta.weight.unwrap(), 3);

        assert!(pages[2].clone().next.is_none());
        assert!(pages[2].clone().previous.is_some());
        assert_eq!(pages[2].clone().previous.unwrap().meta.weight.unwrap(), 2);
    }

fails because pages[1].next is actually None, which shouldn't be the case as it should be pointing to pages[2] and the next previous/next are inverted. Once this is fixed, the code should be working with weight with the correct semantic prev/next elements

Keats commented 6 years ago

After the discussion about sorting, I've made the smallest fix in #3 I think

Keats commented 6 years ago

Can you put the README commit in a new PR?