MWPW-161845: basic analytics on mas cards #3206
Conversation
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## stage #3206 +/- ##
=======================================
Coverage 96.37% 96.37%
=======================================
Files 245 245
Lines 56716 56733 +17
=======================================
+ Hits 54661 54678 +17
Misses 2055 2055 ☔ View full report in Codecov by Sentry. |
| @@ -237,6 +251,7 @@ export async function hydrate(fragmentData, merchCard) { | |||
| merchCard.removeAttribute('badge-color'); | |||
| merchCard.removeAttribute('badge-text'); | |||
| merchCard.removeAttribute('size'); | |||
| merchCard.removeAttribute('daa-ll'); | |||
There was a problem hiding this comment.
why would there be a daa-ll attribute there?
There was a problem hiding this comment.
will fix to 'daa-lh'
| @@ -90,6 +90,7 @@ export class MerchCard extends LitElement { | |||
| reflect: true, | |||
| }, | |||
| merchOffer: { type: Object }, | |||
| analyticsId: { type: String, attribute: 'daa-lh', reflect: true }, | |||
There was a problem hiding this comment.
you could reuse constant from hydrate for daa-lh
There was a problem hiding this comment.
I really think that we should not use. daa-* to avoid conflict with martech, since here it is merely a metadata to be consumed by CCD consumer.
There was a problem hiding this comment.
the plan is that it will be not just metadata, but also a field for martech (so the conflict is intended)
e.g. for plans these attributes will serve the analytics values for martech
| merchCard.querySelectorAll(`a[data-analytics-id]`).forEach((link) => { | ||
| link.setAttribute(ANALYTICS_LINK_ATTR, `${link.dataset.analyticsId}--${cardAnalyticsId}`); |
There was a problem hiding this comment.
| merchCard.querySelectorAll(`a[data-analytics-id]`).forEach((link) => { | |
| link.setAttribute(ANALYTICS_LINK_ATTR, `${link.dataset.analyticsId}--${cardAnalyticsId}`); | |
| merchCard.querySelectorAll(`a[data-analytics-id]`).forEach((link, index) => { | |
| link.setAttribute(ANALYTICS_LINK_ATTR, `${index}-${link.dataset.analyticsId}`); |
There was a problem hiding this comment.
i think dubbing the card lh is not at mas responsibility scope. However it's nice for everyone to get link index within the card
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
| @@ -90,6 +90,7 @@ export class MerchCard extends LitElement { | |||
| reflect: true, | |||
| }, | |||
| merchOffer: { type: Object }, | |||
| analyticsId: { type: String, attribute: 'daa-lh', reflect: true }, | |||
There was a problem hiding this comment.
I really think that we should not use. daa-* to avoid conflict with martech, since here it is merely a metadata to be consumed by CCD consumer.
| @@ -222,6 +225,17 @@ export function processCTAs(fragment, merchCard, aemFragmentMapping, variant) { | |||
| } | |||
| } | |||
|
|
|||
| function processAnalytics(fragment, merchCard) { | |||
There was a problem hiding this comment.
I think we need a unit rest for this.
Add basic analytics to mas cards
Resolves: https://jira.corp.adobe.com/browse/MWPW-161845
Test URLs: