Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Container] Create a Container v2 with the correct JSON output #1407

Open
stefanseifert opened this issue Feb 18, 2021 · 7 comments · May be fixed by #1408
Open

[Container] Create a Container v2 with the correct JSON output #1407

stefanseifert opened this issue Feb 18, 2021 · 7 comments · May be fixed by #1408
Labels
in progress Work is underway on implementing this fix or enhancement.
Milestone

Comments

@stefanseifert
Copy link
Contributor

stefanseifert commented Feb 18, 2021

The JSON output of the Container v1 looks as follows (as it inherits from the Responsive Grid component JSON):

{
	"columnClassNames": {
		"responsiveimage_933266850": "aem-GridColumn aem-GridColumn--default--12",
		"text_1649938976": "aem-GridColumn aem-GridColumn--default--12",
		"text": "aem-GridColumn aem-GridColumn--default--12",
		"title": "aem-GridColumn aem-GridColumn--default--12"
	},
	"columnCount": 12,
	"gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
	"allowedComponents": {
		"applicable": false,
		"components": < ... ]
	},
	":items": { ... },
	":itemsOrder": [ ... ],
	":type": "myapp/components/container"
}
{code}

As some applications might rely on this JSON, we decided to:
* keep the JSON output as it is now for container v1 
* create a new version of the Container component (v2) and adapt the JSON output as follows:

**Simple Container:**
{code}
{
	"id": "container-09eae90b3e",
	"layout": "SIMPLE",
	":items": { ... },
	":itemsOrder": [ ... ],
	":type": "myapp/components/container"
}
{code}

**Responsive Container:**
{code}
{
	"id": "container-09eae90b3e",
	"layout": "RESPONSIVE_GRID",
	"columnClassNames": {
		"responsiveimage_933266850": "aem-GridColumn aem-GridColumn--default--12",
		"text_1649938976": "aem-GridColumn aem-GridColumn--default--12",
		"text": "aem-GridColumn aem-GridColumn--default--12",
		"title": "aem-GridColumn aem-GridColumn--default--12"
	},
	"columnCount": 12,
	"gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
	":items": { ... },
	":itemsOrder": [ ... >,
	":type": "myapp/components/container"
}
stefanseifert added a commit to stefanseifert/aem-core-wcm-components that referenced this issue Feb 18, 2021
@antoantonyk
Copy link

@stefanseifert I really wanted to have the exact JSON model like you mentioned here. I tried to implement the changes based on your PR. But now it is breaking the responsive layout, because the responsive layout component properties(columnClassNames, columnCount, gridClassNames) are missing. Is there any way to include these as well in addition to container properties?

@cshawaus
Copy link

cshawaus commented Aug 20, 2021

Hi,

I also come across this problem on a work project and until last night couldn't see a solid path forward without reimplementing lots of private code. However, I stumbled upon ResponsiveGridExporter which is what ResponsiveGrid uses for its own implementation.

Last night I worked on this and found a way to maintain the responsive grid properties while allowing for custom properties to be available in the model output. The link below shows how the model needs to be structured.

https://github.com/cshawaus/sling-delegation-demo/blob/main/core/src/main/java/com/slingdelegation/core/models/CustomContainerModel.java#L29-L43

There are 3 important things happening in my approach:

  1. Explicitly declare @Exporter to ensure Jackson always generates JSON for our model
  2. Have our model extend the built-in ResponsiveGrid class which has all the private goodies we want
  3. Uses a custom exporter (CustomContainerExporter) which extends ResponsiveGridExporter

Essentially, what I have achieved with this approach is backwards compatibility with AEM's responsive grid while ensuring Sling Delegation remains functional with LayoutContainer. And like magic, the below image shows myCustomProperty available in the default output provided by AEM.

image

Now, with anything there are caveats and so far I have only found one which is SPA root models fail to resolve child :items and always returns empty. This appears to be an issue with Sling Model Factories in HierarchyPageImpl and I currently have a bug ticket open but need to add an SPA example to my demo repo.

As long as you aren't expecting to use the SPA, this solution will work perfectly fine and based on my testing doesn't introduce any degradation to performance. Hopefully this helps you move forward as it certainly solved 50% of my issues after weeks of no progress.

Update for SPA: I have found a solution that ensures both normal and AEM SPA use remain functional.
cshawaus/sling-delegation-demo@c721125

@jckautzmann
Copy link
Contributor

@adobe export issue to Jira project SITES as Story

@github-jira-sync-bot
Copy link
Collaborator

✅ Jira issue SITES-3332 is successfully created for this GitHub issue.

@github-jira-sync-bot github-jira-sync-bot added the in progress Work is underway on implementing this fix or enhancement. label Oct 13, 2021
jckautzmann added a commit that referenced this issue Oct 14, 2021
…ped in model.json Export

- add the ResponsivGrid specific fields to the model JSON output when the responsiveGrid layout is used
@jckautzmann
Copy link
Contributor

The JSON should be as follows for a simple container:

{
	"id": "container-09eae90b3e",
	"layout": "SIMPLE",
	":items": { ... },
	":itemsOrder": [ ... ],
	":type": "myapp/components/container"
}

I see following options how the JSON should look like for a responsive grid container?

Option#1:

{
	"id": "container-09eae90b3e",
	"layout": "RESPONSIVE_GRID",
	"columnClassNames": {
		"responsiveimage_933266850": "aem-GridColumn aem-GridColumn--default--12",
		"text_1649938976": "aem-GridColumn aem-GridColumn--default--12",
		"text": "aem-GridColumn aem-GridColumn--default--12",
		"title": "aem-GridColumn aem-GridColumn--default--12"
	},
	"columnCount": 12,
	"gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
	"allowedComponents": {
		"applicable": false,
		"components": [ ... ]
	},
	":items": { ... },
	":itemsOrder": [ ... ],
	":type": "myapp/components/container"
}

Option#2:

{
	"id": "container-09eae90b3e",
	"layout": "RESPONSIVE_GRID",
	":items": { ... },
	":itemsOrder": [ ... ],
	":type": "myapp/components/container"
}

IMHO option#1 makes the most sense as it has the basic properties of a simple container and adds the responsive grid specific properties.
@stefanseifert @gabrielwalt @vladbailescu WDYT?

@jckautzmann
Copy link
Contributor

After discussing with our team, we decided to:

  • keep the JSON output as it is now for container v1 as we don't want to break existing applications relying on the current JSON
  • create a new version of the Container component (v2) and adapt the JSON output as follows:

Simple Container:

{
	"id": "container-09eae90b3e",
	"layout": "SIMPLE",
	":items": { ... },
	":itemsOrder": [ ... ],
	":type": "myapp/components/container"
}

Responsive Container:

{
	"id": "container-09eae90b3e",
	"layout": "RESPONSIVE_GRID",
	"columnClassNames": {
		"responsiveimage_933266850": "aem-GridColumn aem-GridColumn--default--12",
		"text_1649938976": "aem-GridColumn aem-GridColumn--default--12",
		"text": "aem-GridColumn aem-GridColumn--default--12",
		"title": "aem-GridColumn aem-GridColumn--default--12"
	},
	"columnCount": 12,
	"gridClassNames": "aem-Grid aem-Grid--12 aem-Grid--default--12",
	":items": { ... },
	":itemsOrder": [ ... ],
	":type": "myapp/components/container"
}

jckautzmann added a commit that referenced this issue Oct 20, 2021
…ped in model.json Export

- exclude Allowed Components from the JSON output
@jckautzmann
Copy link
Contributor

Following code can be used as a starting point:
development...issue/1407

@github-jira-sync-bot github-jira-sync-bot changed the title [Container] Missing @Exporter annotation - JSON output skipped in model.json Export [Container] Create a Container v2 with the correct JSON output Oct 20, 2021
@gabrielwalt gabrielwalt added this to the Major milestone Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Work is underway on implementing this fix or enhancement.
Projects
None yet
6 participants