-
Notifications
You must be signed in to change notification settings - Fork 48
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
Support for top and bottom of page events. Combine personalization display notifications together. #1036
Conversation
…mbining notifications" This reverts commit 8a97a0b.
👍 |
.eql({ | ||
propositions: [ | ||
{ | ||
scope: "noView", |
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.
scope: "noView", | |
scope: "noview", |
Needs to be lowercase to match #1027.
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.
Nope, this shouldn't be lowercase. I have added a comment to the previous merged PR too. We shouldn't change the event triggered by the customer. The viewName
will be lowercase only by Personalization component to store/get from cache. Data Collection should not be impacted by the Personalization logic.
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.
That is what I did here: ca33032. The lowercase logic now only resides in the viewCache when saving and matching view decisions.
) | ||
.eql([ | ||
{ | ||
scope: "noView", |
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.
scope: "noView", | |
scope: "noview", |
Needs to be lowercase to match #1027.
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.
Thanks for bringing this up. I've made a different change so that the lowercase scope is only used in the view cache. ca33032
Description
Changes to functionality:
Additionally, this PR includes extensive refactoring of the personalization component. The key changes are:
Related Issue
https://jira.corp.adobe.com/browse/PDCL-9860
Motivation and Context
I had much of this in #1025, but then after some comments from Nina and Jason I was able to make this much more simple. I mistakenly made the updates to the top and bottom branch, but now that it is done, it doesn't make much sense to separate them.
Screenshots (if appropriate):
Types of changes
Checklist: