-
Notifications
You must be signed in to change notification settings - Fork 230
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
WSTEAMA-1412: Add Reverb dependency as the transpiled version #12189
base: latest
Are you sure you want to change the base?
Conversation
metadata: { atiAnalytics }, | ||
} = articleDataNews; | ||
|
||
// @ts-expect-error - only partial data required to manually set 'useReverb' to true |
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.
Will these remain in the codebase for now or is there a timeline to adjust the code so that they're no longer needed?
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.
They will remain there as its only test mock data
fetch(url, options) { | ||
if ( | ||
url === | ||
'https://mybbc-analytics.files.bbci.co.uk/reverb-client-js/reverb-3.9.2.js' |
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 this url need to be dynamic to account for future versions of the Reverb dependency?
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.
Good shout.
I'll move this to a config file to allow for a more appropriate
approach to updating the version of Reverb if/when we need to.
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.
</script> | ||
<script | ||
async="" | ||
src="https://mybbc-analytics.files.bbci.co.uk/reverb-client-js/reverb-3.9.2.js" |
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.
Same as above
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.
This is a snapshot file and as such it will retain the URL.
</script> | ||
<script | ||
async="" | ||
src="https://mybbc-analytics.files.bbci.co.uk/reverb-client-js/reverb-3.9.2.js" |
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.
And again
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.
The case above applies here too; this is a snapshot file and as such it will retain the URL
hashedId: getAtUserId(), | ||
isSignedIn: false, |
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.
Do we need the same logic as above (lines 480-481)?
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 highlighting this Karina.
I reckon we should. Let me take a look at the existing logic
to confirm what informed this.
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've set this to false
in both buildReverbAnalyticsModel and buildReverbPageSectionEventModel.
This is informed by the documentation here.
I've checked the beacons and confirmed that idclient
is still reported in beacons
fired to ATI/Piano.
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.
When I was working on my personalisation project, I had set it to be !!hashedId
- see #11353 (comment)
@gavinspence is this something we would like to put in place now, or shall we leave it for later?
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 the context. I've responded in the thread in 11353 for continuity.
I'll hold off merging pending steer from @gavinspence.
…nto reverb-dependency-minified
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.
Stellar work!
…nto reverb-dependency-minified
…nto reverb-dependency-minified
…nto reverb-dependency-minified
Resolves JIRA [WSTEAMA-1412]
Overall changes
Integrate the hosted version of Reverb into the page for tracking purposes.
Code changes
Testing
Helpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines