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

Move chartbeat out of unmount and refactor to avoid unwanted reloading #3621

Closed
1 task done
dr3 opened this issue Sep 8, 2019 · 4 comments · Fixed by #4793
Closed
1 task done

Move chartbeat out of unmount and refactor to avoid unwanted reloading #3621

dr3 opened this issue Sep 8, 2019 · 4 comments · Fixed by #4793
Assignees
Labels
ws-articles Tasks for the WS Articles Team

Comments

@dr3
Copy link

dr3 commented Sep 8, 2019

Is your feature request related to a problem? Please describe.
Our current chartbeat code is run inside the article main. This means it gets unmounted when you navigate to a new article (due to the loading screen).

Currently the unmounting logic causes issues as it reinitialises charbeat by reloading the script again.

It also doesnt call window.pSUPERFLY.virtualPage in the right place, calling it on unmount instead of when a new article is loaded.

Describe the solution you'd like
Move the chartbeat logic into pageWrapper or contexts, (or anouther new layer above withLoading to avoid being unmounted.

Possible sollution:
Move it into UserContext (or anouther context), creating a useChartbeat function which takes config as an argument. When useChartbeat is first called it renders the scripts using helmet (and never unmounts them).

If the config argument changes (data changes), the useChartbeat function will call window.pSUPERFLY.virtualPage for you.

We need to be very careful to use the useEffect hook properly. The config object should only change when data changes, not on renders. If this is done wrong, even if config is regenerated the same across the renders, useEffect will see it as a new object, calling window.pSUPERFLY.virtualPage too many times

Testing notes
This issue will require a complete testing of chartbeat functionality on articles pages when this refactor is done.

Dev insight: Will Cypress tests be required or are unit tests sufficient? Will there be any potential regression? etc

  • This feature is expected to need manual testing.

Additional context
Add any other context or screenshots about the feature request here.

@dr3 dr3 added Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. ws-articles Tasks for the WS Articles Team simorgh-core-stream labels Sep 8, 2019
@jamesdonoh
Copy link
Contributor

Allthough the PoC above was merged the next steps for this issue are to determine if there is a more optimal solution and refactor as required.

@jroebu14
Copy link

Needs investigation to see if the current chartbeat implementation is up to scratch. Worth checking test coverage too. A senior could pair with someone on this

@jroebu14 jroebu14 removed the Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. label Nov 18, 2019
@samora samora assigned samora and unassigned samora Nov 20, 2019
@jroebu14
Copy link

@jamesdonoh

Myself and Andrew have been pairing on this issue and we would like some context around why it is necessary to use the window.pSUPERFLY.virtualPage API for onward journeys? Can we instead just rely on the IIFE with the async chartbeat script tag being called on every article render?

@rhenshaw56
Copy link
Contributor

Myself and Andrew have been pairing on this issue and we would like some context around why it is necessary to use the window.pSUPERFLY.virtualPage API for onward journeys? ?

@jroebu14 IIFE with the async chartbeat script tag just initialises chartbeat. If this wasn't an SPA we could do without window.pSUPERFLY.virtualPage as their script sufficiently handles this for non-spa sites but it's recommended in their docs to do this to update most of the config values that have changed, in this case title, referrer, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ws-articles Tasks for the WS Articles Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants