Initial GTM Migrations #456
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| useEffect(() => { | ||
| setTimeout(() => { | ||
| setTimerReached(true); | ||
| }, 30000); | ||
| const routeChanged = () => { | ||
| setSecondPageReached(true); | ||
| }; | ||
|
|
||
| router.events.on("routeChangeComplete", routeChanged); | ||
| return () => { | ||
| router.events.off("routeChangeComplete", routeChanged); | ||
| }; | ||
| }, [router.events]); | ||
|
|
||
| useEffect(() => { | ||
| setIsEngaged(secondPageReached && timerReached); | ||
| }, [secondPageReached, timerReached]); |
There was a problem hiding this comment.
does this have to be two effects? From what I can tell from the logic, we are checking
- if the second page was reached
- if they've been here for 30 seconds
Since the timer isn't being reset anywhere, we can just put setIsEngaged into the timeout instead right? this removes extra state and an extra useEffect. something like this pseudo code below. what do you think?
useEffect(() => {
const timeout = setTimeout(() => {
setIsEngaged(secondPageReached);
}, 30000);
const routeChanged = () => {
setSecondPageReached(true);
};
router.events.on('routeChangeComplete', routeChanged);
return () => {
clearTimeout(timeout);
router.events.off('routeChangeComplete', routeChanged);
};
}, [router.events, secondPageReached]);There was a problem hiding this comment.
Doesn't quite match the criteria for tracking the engagement metrics.
With the current logic the timeout continues through page navigation and will trigger the tracking events when
- The User has spent 30 seconds on the site
- The user has visited at least two pages
With this proposed implementation the timeout resets on page navigation and the user would have to spend the full 30 seconds on the second (or any subsequent) page visited to be marked as an engaged viewer. This would cause a rapidly navigating user to not be marked as an engaged viewer.
| if (!isEngaged) return; | ||
| // Trigger engagement view events here | ||
| sendEngagedView(); | ||
| }, [isEngaged]); |
There was a problem hiding this comment.
Do we have to return an isEngaged? Would it be better if we just sendEngagedView in the hook where we are setting engaged?
There was a problem hiding this comment.
I think this mainly comes down to which solution you perceive as being more readable. I think the current solution keeps the hook readable by isolating the tracking logic from the tracking events.
But like I said, it's mostly personal preference at this point.
Changes
Merge Checklist:
Follows the same checklist as the linked Next PR
Testing checklist:
Next PR: https://github.com/gravitational/next/pull/2460
Blog PR: https://github.com/gravitational/blog/pull/535