CIF-1182 - Use the same property type in components with product DnD #221
Conversation
- make it possible to configure the property type to be saved when drag'n'dropping a product
Codecov Report
@@ Coverage Diff @@
## master #221 +/- ##
============================================
- Coverage 60.14% 59.82% -0.33%
+ Complexity 541 531 -10
============================================
Files 148 147 -1
Lines 4393 4348 -45
Branches 746 736 -10
============================================
- Hits 2642 2601 -41
+ Misses 1660 1659 -1
+ Partials 91 88 -3
Continue to review full report at Codecov.
|
| accept="[.*]" | ||
| groups="[product]" | ||
| propertyName="./selection"/> | ||
| propertyName="./multiDropTarget->@selection"> |
There was a problem hiding this comment.
Why needs the product teaser the "multiDropTarget"?
There was a problem hiding this comment.
Because we implement that DnD "save" feature in the MultiFieldDropTargetPostProcessor so we don't have to introduce another post-processor. I added the multiple parameter to control the ADD or REPLACE behaviour, so the teaser uses false so DnD'ing another product replaces the selection.
There was a problem hiding this comment.
+1 for keeping everything in on post-processor. But this looks cumbersome somehow because it is not "AEM standard".
There was a problem hiding this comment.
I wonder if we can find a way to get both property flavors: propertyName="./selection" and propertyName="./multiDropTarget->@product" working?
The first format is the AEM "standard" and ideal for components which do only accept d&d of one product this should be used.
There was a problem hiding this comment.
Well if we want to allow the "standard" DnD AND the choice of selectionId (sku, path, slug), we must have a way to "detect" that special DnD feature. A possible alternative to what is being done here could be:
propertyName = "./selection"
<parameters
selectionId="multiDropTargetValue->sku@selection">
We need 2 things:
- we must "indicate" that this POST must be processed by the
MultiFieldDropTargetPostProcessor(so we need a specialStringsomewhere to "activate" the post processor) - we have to indicate which property will hold the value. This is required because the DnD POST sets all properties in the same HTTP form, so we have to make sure we know which property must be modified before saving.
So I think it's possible, but would add more complexity to the post-processor, and anyway requires a special configuration in cq:editConfig ... so I don't really see a big benefit. If someone only needs the standard existing DnD, they can just continue to use this.
There was a problem hiding this comment.
Ok, I think I can agree, let's keep the current approach then.
| if (SKU.equals(selectionType) || COMBINED_SKU.equals(selectionType)) { | ||
| propertyValue = StringUtils.substringAfterLast(propertyValue, "/"); | ||
| } else if (SLUG.equals(selectionType)) { | ||
| Resource product = resourceResolver.getResource(propertyValue); |
There was a problem hiding this comment.
The SLUG resolving part must be changed, this only works because of the virtual product bundles are installed by the connector. We must not have any dependency on virtual products within the components project. This is will also very likely break with the new authoring tools.
So maybe two simple reasons to move the entire post-processor to the connector project. That is IMHO the better place since it does something which is related to the authoring tools. Then this code part would also be ok.
There was a problem hiding this comment.
I agree it's something more related to authoring so we can move it in the connector project. But strictly speaking, this doesn't really remove the dependency, because for example the carousel DnD target would still be defined with ./multiDropTarget->@product, so it wouldn't work without the connector.
Note that we could also simply remove the "slug" option: both the carousel and the teaser anyway do not support having product(s) configured by slug, so I only added it to demonstrate it's possible to support that, but we don't have any use case for this.
- extended unit tests
- moved post-processor to cif-connector repo
|
As agreed with the team, the post-processor was moved from this repository to the connector repo, see adobe/commerce-cif-connector#105 |
- renamed property to 'selectionId' to align with product picker
|
Verified |
How Has This Been Tested?
TO BE DONE: extend unit tests
Screenshots (if appropriate):
Types of changes
Checklist: