Navigation spike - combo [WHIT-3342]#4076
Conversation
± or shared_navigation, in future. We're just piggy-backing onto document_collections for the spike.
As that's what we've used in the cherry-picked commits
lauraghiorghisor-tw
left a comment
lauraghiorghisor-tw
left a comment
There was a problem hiding this comment.
LGTM but a bit difficult to wrap my head around some bits - left some comments.
It would be nice to have in the spike output (Jira card or even better, an outcome section on the confluence decision log):
- A little flow/diagram/stickies/something outlining the exact interaction with pub api, the payloads in whitehall, and the eventual representation in content store after link expansion; using the last version of the terminology (I think your card comment has the latest?)
- Tag Sally about the terminology - I am sure she'd have thoughts.
- A more detailed explanation/ the why of some of the decisions around the new expansion rule and why current rules are insufficient.
| ) | ||
| end | ||
|
|
||
| it "expands all member documents under the collection" do |
There was a problem hiding this comment.
I am not sure I am reading this test correctly. Can I check my understanding?
We want to expand the links on a parent or child document, so we show smth like:
{
details: {},
links: {
shared_navigation: [
{
content_id: <>,
links: {
documents: [
{
content_id: <self/this LP>,
title: <>,
base_path: <>
},
{ <some other doc in nav> }
]#end documents
}#end shared navigation links
}#end shared navigation item -> can we have multiple?
]#end shared navigation array
}# end links
}
Is that right? The test feels like we are expanding the links on the navigation (collection) itself.
There was a problem hiding this comment.
The other bit I am struggling with, and maybe it's worth writing this out explicitly in the spike output - is on whose publication/put-content/both call we are doing the expansion work - a landing page's or the navigation's?
There was a problem hiding this comment.
And also here, if we need to support non-child documents in the nav, and a document can be added to multiple navs, this shared_navigation link on the doc page would actually be an array of more than one - that would be terribly confusing for figuring out what nav to render. If we ever wanted to inject some nav on non-lp (read: semantic) content types - which honestly, let's not 😅 but I may have read somewhere on some doc that we would consider that.
There was a problem hiding this comment.
Hopefully this is clearer in the revamped spike and PR description over at #4085 😅
As for the shared_navigation being an array - yes, that's a bit uncomfortable. Publishing API convention seems to be that these links are always arrays.
We could consider adding a maxItems: 1 rule, as is done for primary_publishing_organisation?
There was a problem hiding this comment.
I get it now about the behaviour we had on collections vs this.
- Test still reads weird to me 😅
- I like the maxItems idea 👌
On the point about the array actually having multiple navs for a linked page that is not a landing page (child) type - I am thinking we wouldn't enable rendering of any nav in the FE for our old types, so we simply wouldn't consume that expanded links information. We could just use the parent link solution for breadcrumbs if we have to include such pages in nav (end of Q2 concern).
| end | ||
| end | ||
|
|
||
| def document_collection_member_links(collection_content_id) |
There was a problem hiding this comment.
I know it's just a spike - would be nice to write this in a more readable way when we implement, the code duplication between this and the logic in the method that calls it, is a bit dizzying.
| return {} | ||
| end | ||
|
|
||
| reverse_link_type = node.link_types_path.first |
There was a problem hiding this comment.
When we need to support multiple nesting, do we mix in the recursive logic as well?
There was a problem hiding this comment.
Nope - as the links are flat. The nesting (if we do ever do that) would be in the details.menu_items JSON structure, which isn't touched by link expansion / dependency resolution.
|
|
||
| # These expand every member of the reciprocal direct link type from | ||
| # the source link set, instead of only mirroring the edition being expanded. | ||
| REVERSE_LINKS_WITH_MEMBER_EXPANSION = { |
There was a problem hiding this comment.
So we need this because if we used the normal rule, the expanded nav links in a LP content item would include that single LP content item rather than all?
Just looking at a typical collection https://www.integration.publishing.service.gov.uk/api/content/government/statistics/dcms-economic-estimates-gva-2024-provisional.
Can this also be articulated in a bit more detail in the spike output in layman's terms?
There was a problem hiding this comment.
Attempted to explain this in the cleaned up spike at #4085.
| # These expand every member of the reciprocal direct link type from | ||
| # the source link set, instead of only mirroring the edition being expanded. | ||
| REVERSE_LINKS_WITH_MEMBER_EXPANSION = { | ||
| document_collections: :documents, |
There was a problem hiding this comment.
Why is the order of the key and the value reversed here - compared to the old collections? 🤔
There was a problem hiding this comment.
Good question 🤔 Only a spike. But agree for the real implementation it would be nice for this to be consistent with everywhere else in the file.
There was a problem hiding this comment.
I thought maybe the key implies some precedence like doing the expansion for a content item containing links like the key, or something.
But I guess the expansion applies both ways for reverse links? In practice, you wouldn't send those payloads both ways as that would defy the point of the reverse link?
|
Superseded by #4085 |
|
LGTM, I assume we'd need a new link type like |
3 participants
Cherry-picking the best of #4074 and #4075
Jira: https://gov-uk.atlassian.net/browse/WHIT-3342
This application is owned by the Content APIs team. Please let us know in #govuk-content-apis when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.