Umbraco-cms: v8: Missing extension methods to get siblings

[bjarnef]

In v8 it seems IPublishedContent is missing extension methods to get sibling node, e.g. sibling of type or siblings of any type.

You can use e.g. node.Parent.Children() or node.Parent.ChildrenOfType() of do some querying on this, but it won't work if the node is under root level, so it hasn't any parent node.

In v7 there are many extension methods for these.

.Sibling()
.Siblings()
.Siblings<T>()

.PrecedingSibling()
.PrecedingSibling<T>()

.FollowingSibling()
.FollowingSibling<T>()

.Previous()
.Previous<T>()

.Next()
.Next<T>()

In v8 we should probably have extension method as .Sibling(), .Sibling(), SiblingsOfType() like we have e.g, .ChildrenOfType().

[Dave-Mosquito]

+1

[zpqrtbnk]

PR https://github.com/umbraco/Umbraco-CMS/pull/4864

Implements siblings because, thought siblings are just parent's children, it gets complicated indeed when there is no parent.

Does not implement previous/next - that's ... more complicated, because content items don't "where" they are in the list of siblings. Plus, I'm not sure they were really useful. I'd need use cases.

See also: ToIndexedArray() which turns an enumerable of published content into an array of items that support IsFirst, IsEvent etc and could very well be extended with previous/next.

[bjarnef]

Thanks for the PR @zpqrtbnk
Would it also make sense to have a .Sibling() extension method as in v7, when you are only looking to a single node?

We have in v7 projects used .Previous() and .Next() in an paged flow e.g. for some kind of Booking/Ordering/Checkout, but I am not sure what the difference is on e.g.
.Previous() / .Previous<T>() vs .PrecedingSibling()/ .PrecedingSibling<T>()

var prevStep = Model.Content.Previous();
var nextStep = Model.Content.Next();

[PerplexDaniel]

@zpqrtbnk Wouldn't your Siblings() implementation also yield the content itself? Is that intended? I'm not sure about the semantics, but I don't think an IPublishedContent item is technically its own sibling, is it?

That is, if we have IPublishedContent items Parent, ChildA, ChildB, ChildC, I would expect ChildB.Siblings() to only yield ChildA and ChildC, not itself as well. Not sure how it worked in v7 to be honest.

[bjarnef]

@PerplexDaniel I agree with that. Siblings() shouldn't return the node itself, which it didn't in v7 as far I remember. I haven't tested the PR, but from the code it seems it doesn't exclude the node itself.

[bjarnef]

I did a quick test in v7 and both of these methods actually returns the current node as well. I would say that is a bug that it include the node itself.

.Siblings();
.Siblings<IPublishedContent>();

[zpqrtbnk]

Current Siblings implementation does indeed return the current node, because I totally forgot to exclude it - and yes, that's something we should / can fix (should be simple).

I guess the PreviousSibling<T> was a way to get the previous sibling of a given type. Yet... I'd rather not have previous / next at all if that's not too bad. To keep things simple a content currently only knows about its parent, and its children. It does not know "where" it is within its siblings.

Thoughts?

[zpqrtbnk]

Looks like I was too quick to reply: even in v7, Siblings does in fact return all parent's children, including this one. So it's not really siblings. We've discussed it with Bjarke and it looks like the bare minimum which we'd like to implement is:

ParentChildren() returns the children of this document's parent - including this document

Not sure we'd implement Siblings() but if someone needs it, it's quite easy to derive it as doc.ParentChildren().Where(x => x.Id != doc.Id)

What do you think?

And then about previous/next ... again the idea is that, to simplify front-end caches implementation, we don't require that an IPublishedContent knows about its siblings. This is also why the IsEven(), IsOdd(), IsFirst() etc have been removed. Would love to hear about use cases for navigating siblings?

[bergmania]

I agree with @zpqrtbnk. I don't see any good use case for returning all siblings without the current one, as long as I don't know the current node's position between these children.
In all use cases I can think of, I would like to special case handling the current node (e.g. marking active in a list).

[bergmania]

I have updated the PR to reflect the naming ParentChildren().

@bjarnef, do you have any thoughts? Otherwise we will not implement the Siblings methods, but only the ParentChildren() equivalent to Siblings() in V7.

Due to the misleading name in V7, we change the name in V8.

[PerplexDaniel]

Alternatively, a Siblings() + SiblingsAndSelf() would be a fairly descriptive API as well, or just a Siblings(bool includeSelf). With ParentChildren I would be confused as to what it would do for content at the root level as they don't have a parent, yet the ParentChildren() then does return a list of stuff as it just does Content.GetAtRoot().

[bergmania]

@PerplexDaniel, what are the use case of Siblings without the current node?

[bjarnef]

@bergmania can't the method be named Siblings() and SiblingsAndSelf() as @PerplexDaniel suggest.
In v7 when we was using Siblings() we often excluded the current node (the parameter name content).

e.g. you could have some category nodes and on a category page, you want to list other categories (maybe under a parent/master caegory), but I don't want to list the category it self. In v7 you would have to exclude the current category from siblings.

[PerplexDaniel]

@bergmania
As you mention earlier, the node itself usually gets rendered differently and we already have it anyway (otherwise we cannot call .Siblings() or .ParentChildren() on it), so I do not see why it should be returned again.

Off the top of my head I can imagine a scenario where the node itself gets rendered as the main thing, and then there is a list of siblings() for further navigating, e.g.:

@* Main Content *@
<h2>@Model.Content.Title</h2>
<img src="@Model.Content.Image.Url" />

@* Siblings *@
<h3>Related items:</h3>
<ul>
@foreach(var sibling in Model.Content.Siblings()) 
{
    <li><a href="@sibling.Url">@sibling.Title</a></li>
}
</ul>

I'm sure there are plenty of other use cases.

[bergmania]

The reason to return it again, should be to indicate the position between the other nodes.

The only thing I don't like about the name Siblings is that it looks to be same functionality as in V7, but is not.

[bjarnef]

Maybe it is just me, but isn't ParentChildren a silly name - isn't this just siblings, when thinking on a family (but in this case ParentChildren include yourself).

Compare with building an app for a Family Tree and you would like to list your siblings.
Here is an example: https://www.familyecho.com

[bjarnef]

@bergmania it guess there are other parts of the code base, where there are differences.
Maybe it is more about it is being documented on Our 😃
https://our.umbraco.com/Documentation/Reference/Templating/Mvc/querying

[PerplexDaniel]

@bergmania I agree there are certainly situations where you just want all siblings including yourself but likewise I can imagine there are situations where you want to exclude the current node. Since most of the extension methods are for convenience anyway (we can all write our own) it makes sense to include it, in my opinion. This is obviously not a hugely important issue but I'll just add my 2 cents :)

[bergmania]

I have updated the PR to contain both Siblings and SiblingsAndSelf. I added remark to the Siblings _XMLDoc_ about the difference compared to V7.

[zpqrtbnk]

merged