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

Lite ATI Analytics script #12053

Merged
merged 13 commits into from
Nov 7, 2024
Merged

Lite ATI Analytics script #12053

merged 13 commits into from
Nov 7, 2024

Conversation

amoore108
Copy link
Contributor

@amoore108 amoore108 commented Oct 16, 2024

Resolves https://jira.dev.bbc.co.uk/browse/WSTEAM1-1420

Overall changes

  • Adds a dedicated script for sending ATI page view for the Lite site instead of using the tracking pixel, taking inspiration for the recently added Opera Mini extreme script
  • Retains support for when JavaScript is disabled by using the noscript tracking pixel in that instance

Testing

  1. Visit http://localhost:7080/pidgin.lite?renderer_env=live
  2. Confirm that the ATI page view beacon is sent in the 'Network > Fetch/XHR' tab. E.g: Screenshot 2024-11-04 at 10 05 51
  3. Select the 'Img' tab and refresh the page
  4. Confirm that no ATI tracking pixel events are sent
  5. Repeat steps for http://localhost:7080/pidgin/articles/cd14lllvdveo.lite?renderer_env=live
  6. Disable JavaScript
  7. Refresh the pages and confirm that the ATI page view beacon does not appear in the 'Fetch/XHR' tab. It should now appear in the 'Img' tab as it will be using the noscript tracking pixel

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

@amoore108 amoore108 self-assigned this Oct 16, 2024
@amoore108 amoore108 marked this pull request as ready for review October 17, 2024 10:01
@amoore108 amoore108 marked this pull request as draft October 18, 2024 15:00
@amoore108 amoore108 marked this pull request as ready for review November 4, 2024 10:04
Comment on lines +71 to +72
{isLite && addLiteScript(atiPageViewUrlString)}
{!isLite && addOperaMiniExtremeScript(atiPageViewUrlString)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking out loud here... what happens if the user views the lite site on opera mini? 😰

I think we're OK, but just wanted to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be ok. The Lite script is basically identical to the Opera Mini script, so they'll get the same behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And there's no danger of analytics being sent twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be something we'll need to test on Opera Mini.

It was hard to diagnose at the time with the duplicate events, but my theory was that React hydration played a part with react-helmet. The way it behaves is it renders on the server once, then remounts the <Helmet> tags again on hydration. Lite skips out hydration, so in theory it shouldn't.

Its just hard to know how Opera Mini servers behave with this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could literally do the exact same thing as we've done with Opera Mini and track if its been sent already in the window object. I don't think there is a problem with that on Lite, so maybe to be absolutely sure, we just copy the Opera Mini implementation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that would work. I had thought that we might be able to consolidate both opera mini & lite implementations into a single script, but I think that would make things too complicated (and it is already complicated enough! 😰 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I'd prefer to keep them separate even if they're almost the same. The Lite version may be extended in the future to handle click events too.

Copy link
Contributor

@Isabella-Mitchell Isabella-Mitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@amoore108 amoore108 merged commit 74e7db4 into latest Nov 7, 2024
11 checks passed
@amoore108 amoore108 deleted the lite-atianalytics-script branch November 7, 2024 09:40
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