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

feat: update v2 of experimentation engine #595

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Conversation

Copy link

aem-code-sync bot commented Jul 7, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@FentPams FentPams requested a review from ramboz July 8, 2024 15:15
@FentPams FentPams marked this pull request as ready for review July 8, 2024 15:15
@trieloff
Copy link
Contributor

A 82-file PR is a bit much, isn't it?

@FentPams
Copy link
Collaborator Author

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 :)

@FentPams FentPams marked this pull request as draft July 15, 2024 16:06
@ramboz
Copy link
Collaborator

ramboz commented Jul 15, 2024

@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 src/* files and the changes to scripts.js

@@ -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-"]')
Copy link
Collaborator

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.

Copy link
Collaborator Author

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
Comment on lines 571 to 577
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')) {
Copy link
Collaborator

@ramboz ramboz Jul 23, 2024

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

Copy link
Collaborator Author

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants