-
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
WSTEAM1-1514: Lite Site Piano Analytics - Spike #12332
base: latest
Are you sure you want to change the base?
Conversation
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.
A general comment - we mentioned that we wanted to add click tracking to the LiteSiteCTA button - could we carve it out into a separate PR (so that it could be merged sooner, and we can start getting metrics), or does it need to be included in this one?
|
||
return ( | ||
<div css={getItemCss({ dir, size })} dir={dir}> | ||
<a | ||
css={[styles.link, size === 'default' && styles.defaultLink]} | ||
href={href} | ||
onClick={clickTrackerHandler} | ||
{...(liteUrl && {'add-lite-tracker-params': liteUrl})} |
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.
Should we make 'add-lite-tracker-params' into a const somewhere which we can reuse in other places?
{...(liteUrl && {'add-lite-tracker-params': liteUrl})} | |
{...(liteUrl && {LITE_TRACKER_PARAM: liteUrl})} |
@@ -145,6 +145,9 @@ const MostRead = ({ | |||
optimizely, | |||
optimizelyMetricNameOverride: 'most_read', | |||
}), | |||
...(isLite && { | |||
liteSiteUrl: 'liteSiteUrl', |
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 know this is just a spike, but what is this value supposed to be - the .lite version of the page it's on?
|
||
export const LITE_TRACKER_FUNCTION = 'liteTrackerFunction'; | ||
|
||
export const liteTrackingScript = () => { |
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.
Again, I know this is a spike, but when it comes to productionising this, can we add a test of some sort (perhaps a snapshot 🤢 ) - to make sure that this functionality doesn't break if we make changes. I don't want us to have to regression test this every time we make a change. (🤞 we never need to change it!!)
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.
Also just a question - are we essentially recreating, but simplifying what the current ATI script does in this function? Is there any other way of doing this without this level of hardcoding in a script? 😬
Don't want to over solutionise since its a spike. But one way I had in my head was creating a top-level listener in the very root
That would listen for all clicks on the page. You could then store your ATI tracking metadata as a data attribute, like:
Then in your
This would save the custom Lite transformer function and adding |
Looks like its possible though from the spike, nice work! |
}`; | ||
}; | ||
|
||
export const constructLiteSiteURL = props => { |
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.
Can we rename this so that it's more representative of what it does?
export const constructLiteSiteURL = props => { | |
export const constructLiteSiteATIURL = props => { |
let clientSideAtiURL = atiURL | ||
.concat('&', 'r=', rValue) | ||
.concat('&', 're=', reValue) | ||
.concat('&', 'hl=', hlValue) |
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.
Beware of injection attack vectors here
export const LITE_TRACKER_FUNCTION = 'liteTrackerFunction'; | ||
|
||
export const liteTrackingScript = () => { | ||
return `function ${LITE_TRACKER_FUNCTION}(event, atiURL){ |
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.
For production we'd need to make sure this code worked on all our supported browsers, it's not been through a transpiler so it may not work on certain browsers - Opera is a pretty good test case.
], | ||
); | ||
} | ||
}, Object.keys(beaconProps)); |
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.
}, Object.keys(beaconProps)); | |
}, beaconProps); |
Is the change you made here functionally equivalent to what was there before the refactor? I think this is passing the names of the props rather than the new dependency beaconProps
?
Resolves JIRA WSTEAM1-1514
Overall changes
A very high-level summary of easily-reproducible changes that can be understood by non-devs, and why these changes where made.
Code changes
Testing
Helpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines