Move Canonical Chartbeat to UserContext #3622
Conversation
rhenshaw56
requested review from
jamesbhobbs,
chris-hinds,
jamesdonoh,
j-pendlebury,
jroebu14 and
sareh
as code owners
| return <UserContext.Provider value={value}>{children}</UserContext.Provider>; | ||
| return ( | ||
| <UserContext.Provider value={value}> | ||
| <Chartbeat config={chartbeatConfig} /> |
There was a problem hiding this comment.
Can we test that Chartbeat is called with chartbeatConfig?
| return <CanonicalChartbeatBeacon chartbeatConfig={config} />; | ||
| }; | ||
|
|
||
| Chartbeat.propTypes = { |
There was a problem hiding this comment.
Can you pull these into simorgh/src/app/models/propTypes please, as we now use them in 2 files :)
| @@ -2,6 +2,6 @@ | |||
|
|
|||
| exports[`Charbeats Analytics Container should call AmpCharbeatsBeacon when platform is amp and toggle enabled for chartbeat on live 1`] = `"amp-return-value"`; | |||
|
|
|||
| exports[`Charbeats Analytics Container should call CanonicalCharbeatsBeacon when platform is canonical, and toggle enabled for chartbeat for local 1`] = `"canonical-return-value"`; | |||
| exports[`Charbeats Analytics Container should call sendCanonicalChartbeatBeacon when platform is canonical, and toggle enabled for chartbeat for local 1`] = `null`; | |||
There was a problem hiding this comment.
Is this snap out of date? not sure if we need it/need to fix it?
There was a problem hiding this comment.
since we're not rendering the canonical here, I just figured we'd refactor the test to be that sendCanonicalChartbeatBeacon is called instead
There was a problem hiding this comment.
Might need to reword the test description then, as I dont get that from the current wording. Also it probably shouldnt be a snapshot test, maybe isNull?
| ...(cookie && { idSync: { bbc_hid: cookie } }), | ||
| }; | ||
| useEffect(() => { | ||
| if (platform !== 'amp') { |
There was a problem hiding this comment.
| if (platform !== 'amp') { | |
| if (isCanonicalAndEnabled) { |
| const { enabled } = useToggle('chartbeatAnalytics'); | ||
| const { env, platform, pageType, previousPath, origin } = useContext( | ||
| RequestContext, | ||
| ); | ||
| const isEnabledForAmp = platform === 'amp' && enabled; |
There was a problem hiding this comment.
| const isEnabledForAmp = platform === 'amp' && enabled; | |
| const isAmpAndEnabled = platform === 'amp' && enabled; |
|
LGTM 👍 Performed regression testing of several assets (Front pages, Articles and Media pages) and all looked good. |
POC of #3621
Overall change: Creates a useChartbeat function
Whats it do?:
chartbeat.jsscript from being mounted and unmounted all the timeNB: the 'UserContext' probably isnt the right place to put this, but as a POC its fine :)
Whats happening?
If you visit
http://localhost.bbc.com:7080/news/articles/c0g992jmmkkoFirst you see a console message
This is rendering the script tag, and sending the first beacon
Then, if you navigate you see
This is the calling of
window.pSUPERFLY.virtualPagewith the new config