adobe / aem-core-wcm-components

Standardized components to build websites with AEM.
https://docs.adobe.com/content/help/en/experience-manager-core-components/using/introduction.html
Apache License 2.0
744 stars 750 forks source link

[Tabs][Carousel] The Json export doesn't work properly for nested Tabs and Carousels #298

Closed andreeadracea closed 5 years ago

andreeadracea commented 6 years ago

The items elements are not exported properly in Json when having nested tabs. (CQ-4253492)

jckautzmann commented 5 years ago

We should first define how the JSON should look like. I see the following options:

Option 1

We introduce a tabs object with the tab item specific properties (for now only the tab title). The :items object consists of all the nested models JSON.

The JSON would look as follows:

{
    "tabs": [
        {
            "title": "Tab 1"
        },
        {
            "title": "Tab 2"
        }
    ],
    ":itemsOrder": [
        "item_1",
        "item_2"
    ],
    ":type": "weretail/components/content/tabs",
    ":items": {
        "item_1": {
            "columnCount": 12,
            "columnClassNames": {},
            "gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
            ":items": {},
            ":itemsOrder": [],
            ":type": "wcm/foundation/components/responsivegrid"
        },
        "item_2": {
            "columnCount": 12,
            "columnClassNames": {},
            "gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
            ":items": {},
            ":itemsOrder": [],
            ":type": "wcm/foundation/components/responsivegrid"
        }
    }
}

Advantages:

Disadvantages:

Option 2

We introduce a specific node for the tab item which:

The JCR structure would look as follows:

tabs
    item1
        responsivegrid
            text
            image
…

The JSON would look as follows:

{
    ":itemsOrder": [
        "item_1",
        "item_2"
    ],
    ":type": "weretail/components/content/tabs",
    ":items": {
        "item_1": {
            ":type": "weretail/components/content/tabs/item",
            "title": "Tab 1",
            "container": {
                "columnCount": 12,
                "columnClassNames": {},
                "gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
                ":items": {},
                ":itemsOrder": [],
                ":type": "wcm/foundation/components/responsivegrid"
            }
        },
        "item_2": {
            ":type": "weretail/components/content/tabs/item",
            "title": "Tab 2",
            "container": {
                "columnCount": 12,
                "columnClassNames": {},
                "gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
                ":items": {},
                ":itemsOrder": [],
                ":type": "wcm/foundation/components/responsivegrid"
            }
        }
    }
}

Advantages:

Disadvantages:

WDYT?

vladbailescu commented 5 years ago

Another option would be to wrap the exported items in a class that adds the extra properties we need and inline/unwrap the original exported model.

The JSON would look like

{
    [..],
    ":itemsOrder": [
        "item_1",
        "item_2"
    ],
    ":type": "weretail/components/content/tabs",
    ":items": {
        "item_1": {
            // These are coming from the wrapper
            "title": "Tab 1",
            // These are coming from the wrapped model
            "columnCount": 12,
            "columnClassNames": {},
            "gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
            ":items": {},
            ":itemsOrder": [],
            ":type": "wcm/foundation/components/responsivegrid"
        },
        "item_2": {
            "title": "Tab 2",
            "columnCount": 12,
            "columnClassNames": {},
            "gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
            ":items": {},
            ":itemsOrder": [],
            ":type": "wcm/foundation/components/responsivegrid"
        }
    }
}

We could use @JsonUnwrapped to inline wrapped model properties.

gabrielwalt commented 5 years ago

TLDR: I'm for Option 3.

Option 1 I like the simplicity of this option, but the way the tabs and the items work is not very consistent: one is an array and the other an object. Would it make sense that we use the same indexes for the tabs?

{
    ":itemsOrder": [
        "item_1",
        "item_2"
    ],
    ":items": {
        "item_1": {...},
        "item_2": {...}
    },
    "tabs": {
        "item_1": {
            "title": "Tab 1"
        },
        "item_2": {
            "title": "Tab 2"
        }
    ],
    ":type": "weretail/components/content/tabs"
}

Option 2 I like the clarity of this option, but it's adding one more resource type. It would require customers to create a relatively useless proxy component just for the items. Would it make sense to skip that resource type property?

{
    ":itemsOrder": [
        "item_1",
        "item_2"
    ],
    ":items": {
        "item_1": {
            "title": "Tab 1",
            "container": {...}
        },
        "item_2": {
            "title": "Tab 2",
            "container": {...}
        }
    },
    ":type": "weretail/components/content/tabs"
}

Option 3 I like the compromise between simplicity and clarity, and it's consistent with the fact that the Tabs are adding the title property to the Layout Container component. Theoretically, it should be the model of the Layout Container that should provide the title in its JSON, but for the components that don't provide a title, it's good if the model of the Tabs can inject that. However, that should only happen when the component's model doesn't output any title, in case the component's model implemented some business logic for the title.

Finally, we should also apply the same architectural concept to the Carousel component.

jckautzmann commented 5 years ago

The following commit implements option 3 with item specific properties (itemTitleand itemPath): https://github.com/adobe/aem-core-wcm-components/pull/365/commits/adb1ecbb8aab5a2f893d65e92a1c9cd1e0805468