Q A

Fixed Issues? Fixes #1031

Patch: Bug Fix? yes

Minor: New Feature? yes

Major: Breaking Change? no

Tests Added + Pass? Yes

Documentation Provided Yes (code comments)

Any Dependency Changes? no

License Apache License, Version 2.0

Q	A
Fixed Issues?	Fixes #1031
Patch: Bug Fix?	yes
Minor: New Feature?	yes
Major: Breaking Change?	no
Tests Added + Pass?	Yes
Documentation Provided	Yes (code comments)
Any Dependency Changes?	no
License	Apache License, Version 2.0

This PR is intended to resolve #1031 and largely resolves #999 (although, the OOTB responsive grid still has some issues).

Despite the discussion in #1031 that indicated a preference to add the method ListItem#getResource() as a simple solution that preserves backwards compatibility; I could not bring myself to write that solution because I feel that it further commits to the idea that ListItems and ContainerItems are the same thing, and would impose restrictions on the API that introduce limitations that do not need to exist.

I think that the suggested change in this PR provides a path forward that will be clearer and open more possibilities, while still maintaining backwards compatibility. I will provide a more complete rational for the the changes:

What is a List Item, and why is it different than a container item?

Let us look at the ListItem interface:

ListItem {
    URL?,
    title?,
    description?,
    lastModified?,
    path?,
    name?
}

Note that everything in this interface is Nullable.

Specifically, the URL field is always set in a ListItem, unless that list item is used as an item in a container, in which case it is never set. It is my opinion that this type of mutual exclusivity in an API is a code smell that indicates that we are potentially dealing with two different concepts.

If we look to the path field and it's description - "the path of this ListItem" - things become even more confusing. What does the "path of this ListItem" mean? is it the path portion of the URL? is it the same as the URL? Is it the resource path? In fact it is the page path for any page based list item, the resource path for a resource based list item, the action link in a teaser CTA (which may or may not be a resource path), or null. The Teaser CTA List Item is an interesting example, because the path in this case does not always have to be the path of a resource in AEM.

In a container list item, it is assumed that the path is the path of the resource; however, the API is not expressive enough to make this contract clear. The ListItem API doesn't even guarantee that the referenced item exists inside of AEM. Indeed, there are numerous feature requests to facilitate including external list items in components such as the List component or Navigation component.

One statement that is (currently) true, is that a ListItem is either acting as a Link (or CTA, or Anchor, or whatever you want to call it), in which case it has a meaningful URL; or, it acts as a container item, in which case it has a path that points to a resource and does not have a meaningful URL.

The two concepts that I believe have been conflated into one are the concept of a "Link" and the concept of a "Container Item". A link has a URL, a container item has a Resource. This is a fundamental difference that should be able to be expressed in the API in a meaningful way.

How to express this difference

The proposed solution is to introduce a new interface for ContainerItem

ContainerItem {
    resource
}

Access to the resource is the only thing that a container item has to provide to be meaningful. The Container interface is then updated to have the method #getChildren() which returns a list of ContainerItems.

The ListItem interface is left alone to preserve backwards compatibility, and Container implementations still provide access to these ListItems; however, the method for accessing them is marked as deprecated in favor of using the new ContainerItem instead.

What about Panel Containers

The PanelContainer is a special case of a Container. Instead of simply returning ContainerItems, they will return PanelContainerItems

in fact, the Container#getChildren() method is generic in that it only promises to return a list of objects that implement the ContainerItem interface; thus, different container component models are free to provide any type of container item, so long as that container item implements or extends the ContainerItem interface (i.e. provides access to the resource that is being contained). Other than the requirement to provide access to the resource that is being contained, which is the one thing that makes a container a container, the component model is free to add any other data to the container item.

This is seen with the PanelContainerItem which provides access to the resource, but also has the additional metadata of the panel title, id, and data layer object. Unlike the current panel implementations, the PanelContainerItem does not pretend to be a Component, when it is not. Under the current implementation, Panels are not components. The Panel Container is a Component, the contained item is a component, but the panel its self is not.

How does this distinction help us going forward?

This distinction allows for tighter, more expressive, and more reliable APIs that are easier to understand. If you are providing a list of resources then you are a container - every item must provide a resource. This is consistent with the concept of containers in AEM, and is consistent with how these items are rendered in Sling (i.e. render the resources).

If you cannot make the promise that these items are resources that can be rendered, then this is not a container and is just a list of things. While it is true that both a Container component and a List component are lists, they are only lists in the dictionary sense of the word "list".

Going forward, it might be useful to be specific about exactly what a ListItem is - perhaps even renaming the ListItem interface to Link, or something similar. Every use of the ListItem, outside of containers, is a link.

Component	Type
List	list of links
navigation	list of links
breadcrumb	list of links
language navigation	list of links
teaser CTAs	list of links

Potentially, in the future, a unified strategy for handling Links will be created, at which point all of these components should return links or lists of links, and certain attributes of the ListItem (such as URL) can be defined as being explicitly not null.

Another benefit of this separation of the concepts of Links and Container Items is that the Container components don't necessarily guarantee that the contents of a container are editable, or even that the resources provided via the ContainerItem are not synthetic or wrapped.

Although all of the current container components in this project use container items to imply that the contained item is an immediate child node of the container node - this isn't really a thing that Sling cares about; so long as you do not need to rely on the JCR path to resolve that resource. This fact is already leveraged in AEM core for the concept of TemplatedResources.

If a container isn't just child nodes, then it can be used to provide entirely synthetic child resources that can (and should) be rendered based on their own resource types. This provides a clear path forward for implementing features such as "Teaser Lists" and "Image Lists" - where the teasers, images, or any other type of content, can be discovered by the model using any strategy and then synthetic resources prepared and returned. Indeed, it becomes relatively easy to implement this functionality. A component such as a Teaser List wouldn't even need custom front end HTL - the layout container in simple mode would suffice so long as the model implementation returned teaser-compatible resources. That isn't to say you wouldn't change the HTL, just that it's not a heavy lift to implement these features in the Core Components or client projects.

Just to give a clear idea of the utility of this approach, here is an example of how a Teaser List component would be implemented

// normal model stuff, presumably implements TeaserList interface which implements the Container interface
public class TeaserListImpl implements Container {

    public List<ContainerItem> getChildren() {
        // find pages, probably using similar logic to the current list component
        // create synthetic resources that specify the resource type = teaser, and set the target to the page
        // return those resources as container items
    }
}

Nov 20 '20 01:11 ky940819

Codecov Report

Merging #1271 (9153f32) into development (3256682) will decrease coverage by 0.15%. The diff coverage is 93.33%.

@@                Coverage Diff                @@
##             development    #1271      +/-   ##
=================================================
- Coverage          85.19%   85.04%   -0.16%     
+ Complexity          1856     1817      -39     
=================================================
  Files                174      174              
  Lines               5235     5108     -127     
  Branches             805      797       -8     
=================================================
- Hits                4460     4344     -116     
+ Misses               315      307       -8     
+ Partials             460      457       -3

Impacted Files	Coverage Δ	Complexity Δ
...nents/internal/models/v1/ResourceListItemImpl.java	`78.57% <ø> (-7.15%)`	`4.00 <0.00> (-1.00)`
...adobe/cq/wcm/core/components/models/Accordion.java	`100.00% <ø> (ø)`	`3.00 <0.00> (ø)`
.../adobe/cq/wcm/core/components/models/Carousel.java	`100.00% <ø> (ø)`	`4.00 <0.00> (ø)`
.../com/adobe/cq/wcm/core/components/models/Tabs.java	`100.00% <ø> (ø)`	`2.00 <0.00> (ø)`
...be/cq/wcm/core/components/util/ComponentUtils.java	`71.69% <72.22%> (+0.26%)`	`19.00 <6.00> (+6.00)`
...e/components/internal/models/v1/AccordionImpl.java	`79.16% <83.33%> (+4.16%)`	`10.00 <3.00> (ø)`
...internal/models/v1/AbstractPanelContainerImpl.java	`94.44% <94.44%> (ø)`	`9.00 <9.00> (?)`
...nts/internal/models/v1/PanelContainerItemImpl.java	`97.14% <97.14%> (-2.86%)`	`11.00 <11.00> (+9.00)`	:arrow_down:
...ents/internal/models/v1/AbstractContainerImpl.java	`92.59% <100.00%> (+1.06%)`	`17.00 <0.00> (-12.00)`	:arrow_up:
...re/components/internal/models/v1/CarouselImpl.java	`100.00% <100.00%> (+8.33%)`	`12.00 <3.00> (+1.00)`
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3256682...55c8a35. Read the comment docs.