link individual blog posts with older/newer

kbucheli commented 7 years ago

I want to have my travel blog posts to link each other. So if you start with the first one one day one, you can go all the journey by following the "Newer" link at the end. Uses the same design as the current pagination of the index page.

The problem I have now is

[Mon Oct 24 22:03:31 2016] [warn] URL broken on /blog/australia/2016/en/2016/07/27/journey-to-australia/index.html: Link with text "
            ← Older
        " has no destination

which is somehow not shown for the pagination links...

coveralls commented 7 years ago

Coverage remained the same at 93.145% when pulling c7becf8443383edf548967f04b20787a84120671 on kbucheli:link_blog_posts into f202b7edc981b5f733a42efd49da0ca296953cc6 on preaction:master.

preaction commented 7 years ago

Excellent! Thanks! I kept thinking we should have something like this, but apparently I kept neglecting to write it down and forgot it.

The warning you're getting is likely because the last page in the blog doesn't have an older page, so there's an <a href="">Older</a> link (the href attribute exists, but is empty). That's likely not what you want, so Statocles warns about it. In order to make the markup more correct, you have to use a conditional. Here's the bit on the blog index page as an example: https://github.com/preaction/Statocles/blob/master/share/theme/default/blog/index.html.ep#L52-L81

Also, what's the t() template function like you use in t('Older')?

Before I can merge, it also needs some tests:

[x] Test that the blog returns pages with their next/prev attributes set. This can be a new test in t/app/blog/pages.t. You just have to make a blog, call pages(), and check the post pages in there have a next and prev attribute (if applicable) and ensure that it's correct.
[x] Test that each default theme has older/newer pagination links on the blog/post.html.ep template. This test goes in t/theme/check.t and also requires that the default themes get updates (share/theme/default/blog/post.html.ep and share/theme/bootstrap/blog/post.html.ep)

If you'd like to have a go at writing the tests, I can help you with any questions you have. Otherwise, it'll be a week or two until I can get to it.

coveralls commented 7 years ago

Coverage remained the same at 93.145% when pulling 0b65a9e52db8f7861397090e1bad73459d130841 on kbucheli:link_blog_posts into f202b7edc981b5f733a42efd49da0ca296953cc6 on preaction:master.

coveralls commented 7 years ago

Coverage remained the same at 93.145% when pulling 4bed9810dad81124add0b55d5eabcb3e8c216c72 on kbucheli:link_blog_posts into f202b7edc981b5f733a42efd49da0ca296953cc6 on preaction:master.

kbucheli commented 7 years ago

Sorry for the t() stuff that leaked in. Wrong copy source ;-). It is from a primitive translation plugin I use for my blog.

Thanks for the hint with the improved links. And I finally understood what site/ is about... The actual site templates delivered are in share/theme/default/. I was now so bold to change both, the default theme template and the site template.

Then I added the suggested tests. Thank you very much for hinting the right place. And as good tests they even found a bug.

preaction commented 7 years ago

Nicely done! Looks like there's just one more minor issue: The changes you made in t/share/theme/blog/post.html.ep (https://github.com/preaction/Statocles/pull/517/files#diff-c62113666913b1410211137d98c0868bR45) don't have the conditional around them to prevent the empty href="" attribute. This is causing the t/plugin/link_check.t plugin to fail because there are two new broken links in the test site (https://travis-ci.org/preaction/Statocles/jobs/170418516#L556). Once that's fixed, this should be good to go.

(as an aside, I really ought to reorganize the tests, increase their isolation so that changes in the blog don't cause plugin tests to fail, make most of the existing tests into explicit integration tests, and document them better, but that's going to be a slow, ongoing project)

kbucheli commented 7 years ago

I updated it accordingly. I stumbled then over another "issue": what we call prev in the code is next in the template and vice-versa. I corrected the rel attributes accordingly.

We cannot change it in the code without breaking backward compatibility :-(

coveralls commented 7 years ago

Coverage remained the same at 93.145% when pulling 1f21c7dbaaee9696589aff566a63f2c135db10ce on kbucheli:link_blog_posts into 8edf6b484de02a8f501c46815f374c3248168c75 on preaction:master.

preaction commented 7 years ago

Yeah. The labels are "Older" and "Newer" because "Next" and "Previous" aren't descriptive enough. But the pagination helper (Statocles::Page::List->paginate) only knows about the order you give it. And, in the blog, the order is newest first. So, to the paginator, "Older" is the "next" page (page number increases as you view older posts), and "Newer" is the "prev" page (page number decreases as you view newer posts).

But to the reader of the page, "Older" is "prev" and "Newer" is "next" in chronological order, except if you want to read them in the order they are on the blog index page then it'd be reverse chronological order and it'd be Older=next/Newer=prev and if you're not confused yet then you just might be insane ;).

But yeah, this is probably something to figure out before 1.0 and API compatibility guarantees. I think it's all really kind of arbitrary, and what we have now makes the most sense despite, yes, being a bit confusing, but if it can be simplified, I'd be greatly interested.

But, this all looks great and ready to go to me. Let me know if it's ready to go from your end, I'll rebase it and merge it and tag a release (I screwed up and had to rewrite some history on master for #514). Thanks for all the help!

kbucheli commented 7 years ago

I am done from my side. Feel free to pull/rebase.

preaction commented 7 years ago

That was a more complicated merge than I thought, but this is now released in v0.081. Thanks!

preaction / Statocles

link individual blog posts with older/newer #517