Research-AEM CS #47
Research-AEM CS #47
Conversation
src/index.js
Outdated
| let isDebugEnabled; | ||
| export function setDebugMode(url, pluginOptions) { | ||
| function setDebugMode(url, pluginOptions) { |
There was a problem hiding this comment.
Do we really need to remove the exports?
There was a problem hiding this comment.
There were errs complaining about export when I tested in my local. Unexpected token 'export' (at clientlib-experimentation.lc-4153cffb83deab11322f9511383920a0-lc.js:209:1)
From my research, the error occurs because my AEM environment doesn't have a module bundler like we had <script src="/scripts/scripts.js" type="module"></script> in EDS project.
Pramod confirmed natively there is no module type, and need to transpile using webpack.
Same reason as the below import ued.js. I commented out for testing, Pramod will have it back in AEM.
There was a problem hiding this comment.
We'd need to adjust this on the AEM side then so we can use the codebase as-is. The exports are needed both for unit testing and also to offer a proper granular API to the consumers.
We could theoretically compile/transpile the JS as needed in AEM using the proper google closure compiler settings in the clientlib config.
There was a problem hiding this comment.
Thanks for the idea, we will try the google closure compiler way :)
src/index.js
Outdated
| @@ -723,9 +697,10 @@ async function getExperimentConfig(pluginOptions, metadata, overrides) { | |||
| config.selectedVariant = toClassName(overrides.variant); | |||
| } else { | |||
| // eslint-disable-next-line import/extensions | |||
| const { ued } = await import('./ued.js'); | |||
There was a problem hiding this comment.
We'll need to keep this 😉 otherwise A/B testing will only ever serve "A" and not "B"
There was a problem hiding this comment.
Same reason as deleting the export key word, no module type support :(
There was a problem hiding this comment.
That's a client lib config we can change. It just defaults to old ES5 code compliance, but we can bump this to ES6 and leverage modules if we want to
src/index.js
Outdated
| // Challenge: experience fragment where CSS class may be different for each variation | ||
| function convertToVariantSelector(selector) { | ||
| const componentType = selector.match(/\.([\w-]+):/g)?.pop()?.replace(/[:.]/g, '') || ''; | ||
| return `.cmp-${componentType}`; |
There was a problem hiding this comment.
I'm not a fan of the hard-coded prefix here… this makes it dependent on the actual markup.
There was a problem hiding this comment.
You are right, my initial purpose is AEM engineer could define what the selector is by changing this function, and minimize other code changes.
After I synced up with Pramod this morning, we found every element could have its unique URI (the text has its url just has text div in its body, the image has its url just point to image, and the mix of them will have its url too), which from my understanding that we can simply fetch the element directly from the variant page 's body > div
But yes, I think the data-experiment-variant-selector could be a good idea, after your review, we will have current version for Pramod to test, if there are cases requiring variant selector to be declared in attributes, I'll further add that logic in 😉
src/index.js
Outdated
| // AEM CS experimentation modifications | ||
| const componentDataList = getAllMetadataAttributes(document, type); | ||
| await Promise.all(componentDataList.map(async (componentMetadata) => { | ||
| const { selector, variantSelector, ...metadata } = componentMetadata; |
There was a problem hiding this comment.
The variantSelector makes this pretty specific to AEM and not generic just based on data-* attributes anymore.
I'm also not convinced that the convertToVariantSelector will work (aside from the hardcoded prefix) it is returning a generic selector and you could have more than 1 on the page that is loaded.
I think leveraging something like data-experiment-variant-selector="…" that you specifically populate via some custom logic in AEM, and then leveraging that in the code instead of the extra variantSelector would likely be more generic and re-usable across different systems.
…the attributes: data-{scope}-* added in callback from runExperiment(),runCampaign(), serveAudience()
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #47 +/- ##
=====================================
Coverage ? 92.63%
=====================================
Files ? 1
Lines ? 774
Branches ? 16
=====================================
Hits ? 717
Misses ? 41
Partials ? 16 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Description
This branch is for research use to track the integration of the experimentation engine within AEM CS
Pramod's repo =>
https://git.corp.adobe.com/pramodh/aemsite/tree/ootb-eds-experiment
Latest version Nov 15th
nov.15.mov
Nov 15 Updates
selectorissue on controller and variant pagebody > divas selector.Nov 15 Issue Explanation:
Pseudo markup:
Two overlaypills will be generated, but all target the first div. Cause is when elements sharing same class (e.g.
.cmp-image), in the case only the first element will be selected.The screenshot:
Solutions Exploration:
Taken Suggestions:
Wrap the selector with more specific way, including its parent/ancestor's ID/class
getSelectorForElement()