Experience Fragment enhancement to allow remote offer from Adobe Target #1424
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1424 +/- ##
============================================
- Coverage 86.98% 86.08% -0.90%
+ Complexity 2475 1939 -536
============================================
Files 217 181 -36
Lines 6623 5355 -1268
Branches 1011 814 -197
============================================
- Hits 5761 4610 -1151
+ Misses 342 299 -43
+ Partials 520 446 -74
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| <sly data-sly-call="${template.placeholder @ isEmpty=!fragment.configured, classAppend='cmp-dd-experiencefragment'}"></sly> | ||
|
|
||
| <script data-sly-test="${properties.enableRemoteOffer && properties.id}" data-sly-set.mboxId="${properties.id}"> |
There was a problem hiding this comment.
I would prefer if we moved the script to a clientlib instead of inlining it. It could activate on page load and process all elements that have a certain data attribute (for example: data-enable-remote-offer).
Also, shouldn't the mboxId leverage component.id instead o properties.id. It's a subtle difference but sometime they could be different.
There was a problem hiding this comment.
@vladbailescu I have moved the inline scripts into clientlib and added the data-attibutes.
Explicetely we are reading the mboxId from properties.id because that field is optional and if someone does not want to use the experience targeting offer, they will not configure the field in which case the backend sling model will generate a random id and which we do not want to use for target. So if any customer wants to use the target offer they will need to provide an id explicitly and the same will be used by both component.id and properties.id
|
|
||
| <script data-sly-test="${properties.enableRemoteOffer && properties.id}" data-sly-set.mboxId="${properties.id}"> | ||
| adobe.target.getOffer({ |
There was a problem hiding this comment.
I would make sure the global adobe.target is available before calling getOffer.
There was a problem hiding this comment.
@vladbailescu added the check to make sure adobe.target is available before it's called.
|
This pull request introduces 1 alert when merging f1d7183 into b192bdc - view on LGTM.com new alerts:
|
|
Kudos, SonarCloud Quality Gate passed!
|
|
@vladbailescu Can you please review and let me know if all looks good? |
|
@vladbailescu Can you please review and let me know if all looks good? |
|
@richardhand @bpauli @jckautzmann |
|
@vladbailescu @richardhand @bpauli @jckautzmann |
|
Kudos, SonarCloud Quality Gate passed!
|
Fixes #1, Fixes #2