OBPIH-5218 Add basic routes tracking with GA #3899
Conversation
|
@mdpearson @jmiranda please review, but hold with merging, because that one might go as a hotfix (which will require to be merged to the hotfix branch) |
There was a problem hiding this comment.
I (still) don't speak React, so I can't comment on most of this code. The webpack changes look reasonable. My main concern about this PR is that we are doubling down on a version of Google Analytics that is going away. My vote would be to bite the bullet and use this ticket as an excuse to migrate to a supported version. Thoughts?
| @@ -82,6 +82,7 @@ | |||
| "react-dropzone": "4.3.0", | |||
| "react-final-form": "3.6.5", | |||
| "react-final-form-arrays": "1.0.6", | |||
| "react-ga": "3.3.1", | |||
There was a problem hiding this comment.
@awalkowiak I believe the GA release supported by this plugin will be turned off July 1. Should we use the following instead?
There was a problem hiding this comment.
@mdpearson yeah I am aware of this. I added the "next step / to do" to the ticket (https://pihemr.atlassian.net/browse/OBPIH-5218?focusedCommentId=140355) to replace it with the ga4 version when we will be forced to do it because what I understood from @jmiranda's comment - we want to wait until it (ga4 tracking setup) "will be forced on done by google for us" (https://pihemr.atlassian.net/browse/OBPIH-5218?focusedCommentId=140307). Replacing the old react-ga with the new one will be easy in that case.
I will recheck tomorrow morning if react-ga4 is backward compatible with od Universal Analytics tracking id. If it is then I'll swap the lib in this PR.
There was a problem hiding this comment.
My main point with respect to any comment I've made about Google Analytics is "I have no f*cking idea what any of this is, how it works, best practices, etc" so someone please investigate and tell us what we need to do.
- Must not break existing UA properties
- Must be able to support GA4 now (if possible without breaking existing properties)
- Should be able to support UA/GA4 without the need of a plugin as recommended here https://stackoverflow.com/questions/64623059/google-analytics-4-with-react
- Should allow configuration to support other analytics tools like matomo, heap, mixpanel, hubspot, etc
There was a problem hiding this comment.
I'll investigate it further.
| @@ -82,6 +82,7 @@ | |||
| "react-dropzone": "4.3.0", | |||
| "react-final-form": "3.6.5", | |||
| "react-final-form-arrays": "1.0.6", | |||
| "react-ga": "3.3.1", | |||
There was a problem hiding this comment.
My main point with respect to any comment I've made about Google Analytics is "I have no f*cking idea what any of this is, how it works, best practices, etc" so someone please investigate and tell us what we need to do.
- Must not break existing UA properties
- Must be able to support GA4 now (if possible without breaking existing properties)
- Should be able to support UA/GA4 without the need of a plugin as recommended here https://stackoverflow.com/questions/64623059/google-analytics-4-with-react
- Should allow configuration to support other analytics tools like matomo, heap, mixpanel, hubspot, etc
| <script> | ||
| window.GA_ENABLED = "<%= gaEnabled %>"; | ||
| window.WEB_PROPERTY_ID = "<%= webPropertyID %>"; | ||
| </script> |
There was a problem hiding this comment.
Is this the recommended way to handle this? I guess the problem is that the React js files are generated at build time so we'd need to slip the properties in at build time and wouldn't have the ability to do so at runtime.
So I'm ok with this ... just want to make sure this follows best practices.
| // eslint-disable-next-line no-template-curly-in-string | ||
| gaEnabled: '${grailsApplication.config.google.analytics.enabled}', | ||
| // eslint-disable-next-line no-template-curly-in-string | ||
| webPropertyID: '${grailsApplication.config.google.analytics.webPropertyID}', |
There was a problem hiding this comment.
Oh shit we do actually do this at build time. 😕
There was a problem hiding this comment.
That scares me a bit since someone would conceivably want to change the config properties at runtime. In fact, this is a bigger issue and they would try to change them, but changing them would have no effect.
There was a problem hiding this comment.
Can we change config properties on runtime? I can put it under getContextApp, to avoid injecting it like that.
| <MainLayoutRoute path="/**/dashboard/:configId?" component={Dashboard} /> | ||
| <MainLayoutRoute path="/**/" component={Dashboard} /> | ||
| </Switch> | ||
| <SwitchWrapper /> |
There was a problem hiding this comment.
Could you explain this component and why we needed to refactor this here?
There was a problem hiding this comment.
In the hook for tracking page views, I am using a useLocation hook from the react-router-dom. The deal with it is that it won't work properly when used at the component using Router (remix-run/react-router#7015 (comment)). Plus in reality, I wanted to use it on the Switch only, that's why I moved out a Switch to a separate component.
|
I am closing this one because of I am going to push version with |
No description provided.