-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: update v2 of experimentation engine #595
base: main
Are you sure you want to change the base?
Conversation
git-subtree-dir: plugins/experimentation git-subtree-split: 638e0db53a4fa7886ef9713acc853fc7b02c76be
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
A 82-file PR is a bit much, isn't it? |
Hi @trieloff, thank you for reviewing. The file changes are mostly come from the update on v2 experimentation plugin, which is a rewrite on v1 engine. Here is the link for reference - adobe/aem-experimentation#28. The features from v2 has been tested, which can be integrated with the project well. I will keep this pr as a draft before we finalizing the v2. Thank you :) |
@trieloff It's a bit of a limitation from subtree/submodules… we have to pull the whole tree, so you end up with all the test and config files in the PR, but the only interesting bits are really the |
scripts/scripts.js
Outdated
@@ -38,7 +38,7 @@ window.hlx.plugins.add('performance', { | |||
|
|||
window.hlx.plugins.add('experimentation', { | |||
condition: () => document.head.querySelector('[name^="experiment"],[name^="campaign-"],[name^="audience-"]') | |||
|| document.head.querySelector('[property^="campaign:-"],[property^="audience:-"]') | |||
|| document.head.querySelector('[property^="campaign:-"],[property^="audience:-"],[property^="campaign-"],[property^="audience-"]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, it's <meta property="campaign:-…">
or <meta name="campaign-…">
, property without :
is something I haven't encountered yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I added that because I was thinking the user could omit the :
, and declare as Audience Desktop
. Then I realized it is better to have the norm that :
should be required. I will revert this.
For <meta property="campaign:-..>
, I am going to remove the hyphen here too, and only keep [property^="campaign:"],[property^="audience:"]
. As the hyphen comes from the space, and should not considered as the condition.
f7c45aa fix: Support Flexible Parsing (#37) f29dd80 fix: failing tests following refactoring 9db77eb fix: audience checkpoint name 7dc74e1 feat: prepare data for RUMv2 collection f6ccbcd fix: update getAllMetadata function (#33) ae769a2 feat: Supporting Naming Variants in Page Expereimentation (#31) git-subtree-dir: plugins/experimentation git-subtree-split: f7c45aa70429cb111599cd5a257e22a0d336f77e
scripts/scripts.js
Outdated
if (ev.detail.element.classList.contains('hero')) { | ||
const parent = ev.detail.element.parentElement.parentElement; | ||
[...ev.detail.element.children].reverse().forEach((el) => parent.prepend(el)); | ||
ev.detail.element.remove(); | ||
decorateBlocks(parent); | ||
loadBlocks(parent); | ||
} else if (ev.detail.element.classList.contains('block')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably remove this condition here and just keep the default block decoration below.
We'll eventually just replace this by a "decoration function" option on the plugin config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should remove the special handling. While current fragment experiment doesn't support correctly decorating hero blocks, could we move this after we can safely handle it?
942948d Update: Extract customized names from 'Experiment Variant Name(s)' in page metadata (#41) 5f942a2 feat: Add decoration handler for handling fragment decoration (#39) 9b4478d feat: Central host UI files (#40) 9db8662 feat: support customized variant name for fragment variants (#36) c9ff2c0 fix: only fetch pathname to fix url matching issue (#38) git-subtree-dir: plugins/experimentation git-subtree-split: 942948d6334265d74d2bfa1342ec015a4c0c64ab
This PR leverages the upcoming v2 of the experimentation engine (https://github.com/adobe/aem-experimentation).
This will cover:
and support use cases for:
Test URLs:
Before: https://main--helix-website--adobe.aem.page/
After: