-
Notifications
You must be signed in to change notification settings - Fork 287
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: EDS personalization using AEP Segments #54
base: main
Are you sure you want to change the base?
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
@@ -0,0 +1 @@ | |||
[{"id":"a44525f4-e115-41d3-a650-eaad3fa2a458","name":"Franklin Feedback Funnel State Lapsed Segment","description":"audience whose membership is lapsed"},{"id":"609b90e4-5306-4c37-b64b-e3026ad1f768","name":"Franklin Feedback photoshop Segment"},{"id":"e52f5a05-0f03-46e7-96a3-74446971af8e","name":"Franklin Feedback Interests segment"},{"id":"c2ba55f2-b327-4c5e-84b0-49df613cd71b","name":"Email Opened Segment"},{"id":"e0eab159-2464-4f93-b3c9-455ce5b5102a","name":"Test Alina2"},{"id":"78ef77be-bbf3-444d-bd15-f51043ce1f8f","name":"All test profiles"},{"id":"f8eeb1c4-42f0-4db5-a209-dc27c1224f23","name":"SitesNewsletterAudience"},{"id":"c891dba5-b7d0-4962-8152-f2463ebf408c","name":"ExpSuccess country segment"},{"id":"60c8669b-b9be-4242-b0ef-c3175808187b","name":"Visited Surf Product Detail Pages"},{"id":"de150daf-4bc5-4132-a550-dcea643cecee","name":"Female Surfers"},{"id":"f1972801-fd98-40b4-a313-18f1a3c84acb","name":"AlinaSegment"},{"id":"12af8939-12f2-4b41-a961-dba44b1e02d6","name":"SitesNewsletterv2"},{"id":"1ff20223-5239-4370-b887-6e69c28e5d28","name":"Cedric Huesler","description":"Person ID equals [email protected]"},{"id":"ef677d6f-44f9-4ac7-8827-c9ff745d1658","name":"Latha","description":"Latha Ramasamy"},{"id":"93f15377-5d55-4f1d-96f8-729ca4308d09","name":"Damian Test"},{"id":"50c89ea1-a3f8-4827-a61f-01ae6711c572","name":"AndreiP-segment"},{"id":"301fb79b-98ba-49ac-aafd-1b3b9e9066c2","name":"Demo segment"},{"id":"f417df3c-dbb0-40eb-8719-3d769d66917f","name":"Subscription - KFTest","description":"test (system generated segment for subscription - do NOT edit)"},{"id":"8ed561e9-bee2-45cf-988c-584f83d3ab96","name":"People who ordered in the last 30 days","description":"Last 30 days"},{"id":"6d7ac2f9-a806-4b90-a0f2-13cdee438acf","name":"Keara Only"},{"id":"f9aad193-fc81-4d1f-8bd0-7e4dcc6ff8ed","name":"Milestone Updates (All)"},{"id":"1946eedb-5372-42bc-85ad-673e0b6172cb","name":"WKND Adventure - Surfing","description":"A test segment of WKND adventurers that enjoy surfing"},{"id":"0839e05b-efff-485f-bbf3-13a533e15bd1","name":"demo"},{"id":"bbf00d86-ab3e-4e87-b77d-7cff7a4cfd36","name":"DX Summit Segment"},{"id":"be0d4fc6-e8f1-49cd-b247-57782f1c58f2","name":"Hemingway-Segment"},{"id":"f119b6e7-1814-4b86-99c6-1fefcca9c638","name":"Customer Journey"}] |
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.
Since this file is supposed to be cached by the sync action, I'd rather have the GH workflow as part of that PR as well, instead of a static version of the file
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.
@ramboz I don't have access to settings on this repo, can you please help setting the secrets sent to you?
plugins/experimentation/src/index.js
Outdated
@@ -63,6 +65,10 @@ export async function getResolvedAudiences(applicableAudiences, options, context | |||
const results = await Promise.all( | |||
applicableAudiences | |||
.map((key) => { | |||
if (key.indexOf(RTCDP_AUDIENCE_PREFIX) !== -1 && options.audiences[RTCDP_AUDIENCE_PREFIX]) { |
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'd be tempted to remove this logic and just have the segment name in the document, and then mapped properly in the audiences Franklin Feedback photoshop Segment
and franklin-feedback-photoshop-segment: () => { … }
If the goal was to have more dynamic mappings without having to specify the exact name, I'd be tempted to use a generic audience resolver then as a fallback, instead of a specific RTCDP one.
if (!options.audiences[key] && typeof options.audiences.default === 'function') {
return options.audiences.default(key);
}
Put probably best opened as a PR against https://github.com/adobe/aem-experimentation/ directly
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'd be tempted to remove this logic and just have the segment name in the document, and then mapped properly in the audiences Franklin Feedback photoshop Segment and franklin-feedback-photoshop-segment: () => { … }
want to use a generic name so that there's no development required to use the feature.
If the goal was to have more dynamic mappings without having to specify the exact name,
yes, this is the goal.
I'd be tempted to use a generic audience resolver then as a fallback, instead of a specific RTCDP one.
Does it make sense if these default/fallback
audience resolve the audience from AEP? Should we document that? How about using the default/fallback audience, which will resolve the RTCDP audience if the rtcdp prefix is there. Does it make sense?
probably best opened as a PR against https://github.com/adobe/aem-experimentation/ directly
Yes, I'll do that. I can't easily verify things in aem-experimentation
repo. So, I've opened here. Once this is complete, I'll submit the changes to aem-experimentation
Should I keep the default/fallback audience implementation in scripts.js or move that also to aem-experimentation
and merge the DEFAULT_AUDIENCE
from aem-experimentation to customer defined audiences?
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.
Does it make sense if these default/fallback audience resolve the audience from AEP?
That would be dependent on the project. they would be responsible of implementing a default audience resolver and can decide how they map this
scripts/scripts.js
Outdated
@@ -32,6 +41,18 @@ const AUDIENCES = { | |||
desktop: () => window.innerWidth >= 600, | |||
'new-visitor': () => !localStorage.getItem('franklin-visitor-returning'), | |||
'returning-visitor': () => !!localStorage.getItem('franklin-visitor-returning'), | |||
rtcdp: async (rtcdpAudience) => { |
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'd use a default
/generic
/fallback
instead so it's not specifically for RTCDP but more generic
scripts/scripts.js
Outdated
@@ -181,7 +202,9 @@ export function decorateMain(main) { | |||
*/ | |||
async function loadEager(doc) { | |||
document.documentElement.lang = 'en'; | |||
establishPreConnections(); |
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 wonder if the preconnect meta is the best option. if we do want to optimize that way, I'd be tempted to go the extra mile and have that directly in the HTTP headers.
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.
yeah, make sense. This can be added to head.html, I'll remove from here.
head.html
Outdated
<link rel="preconnect" href="https://edge.adobedc.net"> | ||
<link rel="preconnect" href="https://adobedc.demdex.net"> |
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 wouldn't do this globally, but only for the pages that actually need it and inject this via the headers.xslx
or metadata.xslx
instead
Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):
Demo:
Test URLs: