Add next/previous navigation

[AWearring]

Added next/previous navigation based upon localgov_guides

[AWearring]

This is working for me locally, but is failing tests - I'm at a loss as to why it is failing https://github.com/localgovdrupal/localgov_blogs/actions/runs/8401705613/job/23010407547?pr=31 Behat\Mink\Exception\ResponseTextException: The text "Next" was not found anywhere in the text of the current page.
I'm my testing locally the nav block is added to the page and has all the correct next/prev links

[stephen-cox]

@andybroomfield to take a look.

Feelings at merge Tuesday is that this should be optional so sites can opt in. There isn't explicit ordering for blogs so the assumption is this will be ordered by date, but this isn't guaranteed.

[AWearring]

@andybroomfield to take a look.

Feelings at merge Tuesday is that this should be optional so sites can opt in. There isn't explicit ordering for blogs so the assumption is this will be ordered by date, but this isn't guaranteed.

They are initially ordered by creation date - however this can be manipulated on the channel node:

[andybroomfield]

I've managed to do a quick test and get it working.

However I'm actully not sure this is the right approach here, trying to use the same method as guides which is a hierarchical content type with explicit ordering, and blog posts that are primarlly in date order, with some being pulled out to be featured. This is making the feature a little complicated to implement as I see that we are tracking new blog posts that are being added just so they can be re-ordered. However sometimes blog posts can be added with different published dates so this would result with them being out of order so having to be reorded each time (On our news pages, often our comms team will have several draft stories being worked on, and they will adjust the date before publishing).

Instead, I belive the better approach is to lean into this being date based content, and make the next / previous links behave as a way of moving to the next / previous blog post based on the date, using the same method as wordpress. I feel this would also be closer to how blog authors would expect it and would like to hear from them on their user needs.

• As a blog reader
• I want to see a link to the previous or next published post
• So I can continue reading posts in date order

If there is a need to re-order a post, the current established pattern in news where we change the post date.

Maybe its worth looking at Nextpre module module?
Or the alternative Flippy
And theres some code here to do a direct implementation.

Things to consider

• Do we need to track blog posts so we can establish order or can we use an entity query?
  So this PR creates a new field on the blog channel where new posts are added, but we need it to always be in date order, then its possible to use the field delta. Alternativly we could use an entity query to find the next and previous post by date, though this might be a little computationally expensive?
• Should we factor in featured blogs posts in the next / previous or ignore them and treat them like date order.

[finnlewis]

Discussing with @ekes and @andybroomfield at Merge Tuesday:

• we're not keen on storing all the blog posts in a field as this could get huge and becomes a performance and usability issue with hundreds or thousands of blog posts.
• Looking at the Nextpre module, that selects the next node based on node id. https://git.drupalcode.org/project/nextpre/-/blob/9.0.x/src/Plugin/Block/NextPreviousBlock.php?ref_type=heads#L234 - this is also not ideal.
• So maybe as @andybroomfield suggests, an entity query to select the next and previous by date / time - then sort by ID for any edge case where multiple nodes are created in the same second. This would be similar to the Nexpre module but better, for our needs.

@AWearring what do you think? Is this something you want to re-work or do you want someone else to dive in a give it a go?

[AWearring]

@finnlewis I'll have a crack at it (time allowing), if I get stuck I will happily pass on to someone else to have a go at it

[AWearring]

Does this work if the directories are src/plugin/block instead of src/Plugin/Block, or would this cause issues with autoloading, static analysis or tests?

Correct @opdavies this would cause issues on file systems that are case-sensitive.

So it should be src/Plugin/Block ?

[millnut]

Does this work if the directories are src/plugin/block instead of src/Plugin/Block, or would this cause issues with autoloading, static analysis or tests?

Correct @opdavies this would cause issues on file systems that are case-sensitive.

So it should be src/Plugin/Block ?

Hi @AWearring yep it should be src/Plugin/Block

[AWearring]

@millnut @andybroomfield @markconroy @stephen-cox @ekes @opdavies
This is so frustrating, when I last installed this it was all working - today using a fresh install after adding block all I get is the block title and no content - can someone take a look and see what I've broken?

[andybroomfield]

Hmm, this seems to have broken since last time I tried it

[andybroomfield]

Have pushed fix for missing links in block. Now just needs the test.
I notice it uses Font awesome, which I seem to be missing so I don't get the chevrons.

[AWearring]

@andybroomfield
Thanks, I started to think I had imagined it working - was too tired to work out what was up.

[AWearring]

Discussion in Merge Tuesday:

How does this become available to existing sites?

For exiting sites, the block will become available, but not placed. For new sites, the block will be placed.

So we'll want to test this!

And add some documentation / release notes as to how to make use of it for existing sites.

Plus some tests, if possible.

Todo:

* test

* review code and approve

* write some tests

* write some docs.

* review for accessibility

See https://docs.localgovdrupal.org/devs/quality-standards/

@finnlewis

• Tested on both existing and new sites and as I expected because the new block is optional it needs to be placed for both
• Tests for the new navigation written and passed
• Docs written see Add initial Readme #35
• I believe the new block template meets accessibility requirements

[markconroy]

Template looks fine to me now, I'll leave others to comment on the backend items.

[finnlewis]

Not urgent, but would be good to this approved soon.

[ekes]

I made two posts on the same day. They don't get any previous next links. I think this is because the granularity of the date in day. If you have two posts on the same day it's not getting to them.

[andybroomfield]

@ekes I think this is becuase the blog date field is a date, not a date time. Simmilar issue in news as it's not got time granuallity. I suggest for this PR we add in checking for a page updated also, and then revise these date fields on blogs and news and convert them to datetime.

[AWearring]

@ekes @andybroomfield If eiether of you have some time to look at adding in a check for page updated that would be awesome - I have very limited time at the moment to look at this so any help would be greatly appreciated.

[andybroomfield]

This is ready for review again @AWearring and @ekes.
I've added a test to demonstrate multiple channels work correctly (next / prev links in the correct channel).

I've also tried to account for blog posts being on the same day. The issue here is that there is insufficent granularity for the localgov_blog_date field which is just a date field. For the moment I've made changes to the way next / prev is queried so it can secondarly use the created time as a sort. The issue here is that can be out of step with the localgov_post_date so we can't just add that as a condition. Instead I've added it so that it includes the current node in the results (account for same day) and then searches the results for the next nid (sometimes it can get thrown by having one earlier). It's not the most efficent and I wonder how this will scale to many blog posts, though it seems to work for now.

If we would rarther skip this and try to change localgov_blog_date to a datetime field, then we just need to drop commit 5492910.