[WORLDSERVICE-643] Implement click tracking on Opera Mini for Canonical #12638
Conversation
…-click-tracking-opera-mini-canonical-work
…-click-tracking-opera-mini-canonical-work
There was a problem hiding this comment.
Let's rename this to LiteTrackingTemplate for consistency with the ReverbTemplate
| __html: `${liteTrackingScripts({ | ||
| onlyOperaMini, | ||
| trackViews, | ||
| trackClicks, | ||
| })}`, |
There was a problem hiding this comment.
I think we can just move the original logic from src/server/Document/Renderers/LiteRenderer.tsx to here
| onlyOperaMini = false, | ||
| trackViews = true, | ||
| trackClicks = true, |
There was a problem hiding this comment.
Let's remove this, we always want to track views + clicks
| onlyOperaMini = false, | |
| trackViews = true, | |
| trackClicks = true, |
There was a problem hiding this comment.
After the changes, the will be used in both CanonicalRenderer and LiteRenderer.
If I understand your comment correctly, removing the conditional rendering (i.e., props here and inside liteTrackingScripts) would cause the scripts to load on all Canonical pages, which might not be what we want?
| // Note: Review if it's possible to assign LITE_ATI only on isLite and Opera Mini devices | ||
| return { [LITE_ATI_TRACKING]: liteHandler, onClick: clickTrackerHandler }; |
There was a problem hiding this comment.
That's a good question. We don't want to double count the clicks.
One way to test this is to check a canonical page on Chrome - and ensure that only 1 event if fired. If 2 events are fired, then we will need to add the check for isLite || isOperaProxy() here.
There was a problem hiding this comment.
Initially my approach was to check for isLite || isOperaProxy() as you suggested. However isOperaProxy doesn't work on the server side due to window being undefined.
@shayneahchoon suggested that useATIClickTrackerHandler could return both - clickTrackerHandler and liteHandler. From my initial testing it shouldn't result in double counting the clicks for Canonical/Lite pages after the hydration due to stopPropagation inside useClickTrackerHandler
Meanwhile if server-rendered HTML version (i.e. Lite/Canonical Opera Mini) is used, event listeners are ignored and lite click tracking would be used.
There was a problem hiding this comment.
Ah yes, I had forgotten isOperaProxy won't work on the server!
|
|
||
| function sendBeaconLite (atiPageViewUrlString) { | ||
| var xhr = new XMLHttpRequest(); | ||
| xhr.open("GET", atiPageViewUrlString, true); | ||
| xhr.withCredentials = true; | ||
| xhr.send(); | ||
| } |
There was a problem hiding this comment.
This also relates to the isOperaProxy not working on the server (As it could be loaded here https://github.com/bbc/simorgh/blob/latest/src/app/components/ATIAnalytics/canonical/index.tsx#L73 ). sendBeaconLite should be defined for Canonical on Opera Mini since it's used by processClientDeviceAndSendLite
There was a problem hiding this comment.
We should probably extract it into a separate script, and include it on canonical + lite pages. I want to try and reduce duplication as best we can. We have a file which already defines it: https://github.com/bbc/simorgh/blob/latest/src/app/components/ATIAnalytics/canonical/sendBeaconLite.ts - so we should ensure that's loaded first, and always available on the window.
There was a problem hiding this comment.
That's a good point, we can just remove isLite check and always load the script for canonical + lite pages
There was a problem hiding this comment.
Perhaps we need to refactor the sendBeaconLite file (+ test) & move it to within the new LiteRenderer template? (@shayneahchoon would you agree?)
…-click-tracking-opera-mini-canonical-work
Resolves JIRA: https://jira.dev.bbc.co.uk/browse/WORLDSERVICE-643
Summary
A very high-level summary of easily-reproducible changes that can be understood by non-devs, and why these changes where made.
Code changes
Developer Checklist
Testing
Ready-For-Test, Local)Ready-For-Test, Test)Ready-For-Test, Preview)Ready-For-Test, Live)Additional Testing Steps
Useful Links