-
Notifications
You must be signed in to change notification settings - Fork 945
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
feat(reactotron-app): Add basic analytics to Reactotron app with react-ga4
#1406
base: master
Are you sure you want to change the base?
Conversation
react-ga4
react-ga4
385def4
to
7528858
Compare
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.
Code looks good, nice work! 👍
I think its worth a UX discussion on the impact of showing that alert on startup.
I suppose some other dev tools do something similar - like this setting in Android Studio: (I don't remember if you get prompted this on first startup or not)
IMO the alert could also use a UI pass if Justin has some bandwidth
// We use this instead of the default alert because we want to style it to match our app. | ||
// We inherit the styles from react-confirm-alert and override them as needed. | ||
// Unfortunately, we can't use styled-components here because react-confirm-alert doesn't support it. | ||
// This also means we have to hard-code the colors here instead of using our theme, which is not ideal. |
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.
just curious - how much is react-confirm-alert
doing for us here?
wondering if its worth the dependency if its difficult to style. could be nice to have full control of the UI, but might not be worth the effort.
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.
Yeah, when i put it in the app, i was thinking it would be useful as-is, but the more i played with it, the more customizations I wound up doing. I'll probably rip this out before i change this pr to no longer be a draft.
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.
This looks good! just left a suggestion 💯
sendAnalyticsEvent({ | ||
category: "overlay", | ||
action: "OverlayDropImage", | ||
label: "Success", | ||
}) |
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 noticed that each sendAnalyticsEvent
has string values for category, action, and label. It may be worth using an enum for these constants. Then it's type-safe and easier to see what is available per key! Thoughts?
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.
Thanks for the suggestion @jonmajorc! Yeah, i need to clean up the types for sending events so we know exactly what we're going to send to the analytics server.
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.
Just a couple of comments. Appears to work as expected.
Low GAFO: I find myself parsing this statement incorrectly half the time:
[analytics] user has changed opt-out status false
The status could be unknown
| opted-in
| opted-out
. Then the statement could be "user has changed status to opted-in
"
|
||
// This is the Google Analytics 4 key for Reactotron | ||
// TODO: Change this to the correct key for production. | ||
const GA4_KEY = "G-WZE3E5XCQ7" |
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 this be an environment var?
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 don't think google analytics keys are considered private or sensitive information
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.
It was less about privacy and more about being able to use CI to build it for the production key and have a dev key locally.
const status = configStore.get("analyticsOptOut") as IOptOutStatus | ||
|
||
if (status === "unknown") { | ||
console.log(`[analytics] user has not opted in or out`) |
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.
Do we want to keep this logging in?
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 think it's safe to leave in. It'll only ever console log once at the beginning of the app boot. It's useful for development to leave it in.
// We also disable analytics if the user has opted out. | ||
useEffect(() => { | ||
const initialize = () => { | ||
const testMode = process.env.NODE_ENV === "test" // we don't want to send analytics events during tests |
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.
What about development? We probably don't care about our usage patterns while developing Reactotron.
Is there another environment besides prod where we would want it enabled?
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.
information will be collected. | ||
</p> | ||
<p> | ||
You can change this setting at any time and by opting in, you can contribute to making |
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.
How do I opt back in or opt out again?
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.
This verbiage isn't final yet, but basically you go to the Electron Menu and "Settings" - it opens a json file in your default editor and you change the json for analyticsOptOut
to true
. Safe the file and reboot reactotron. VIOLLA! no more logging.
@@ -1,3 +1,4 @@ | |||
export const reactotronLogo: string = require("./Reactotron-128.png").default | |||
export const reactotronAnalytics: string = require("./Reactotron-analytics.png").default |
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.
Let's throw this over to Justin and see what he'd like graphic wise.
useEffect(() => { | ||
const timer = setTimeout(() => { | ||
initializeAnalytics() | ||
}, 250) |
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.
What are we waiting for here?
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.
While developing, I found that there was the possibility of this component rendering a couple times. This delay with the clearTimeout is meant to only initialize the analytics once the electron renderer process has completely opened the app and react has stopped reacting. Not sure if there's a better way to handle this other than some memoizing
I also found this discussion from a VERY long time ago #723 where @jamonholmgren had already mocked up a screen and verbiage. |
63aa741
to
b7106f1
Compare
b7106f1
to
1ce3ae7
Compare
@markrickert I rebased this on top of |
This adds google analytics to the reactotron app for basic feature and usage metrics. It's implemented using
react-ga4
but the codebase can easily be switched to another service is we want.The first time the user opens the app after upgrading to this version, they'll be presented with this screen:
Clicking the red "NO" button changes their user setting to opt-out of anonymous analytics. A user that opens the app and clicks "NO" should have zero analytics calls send to google-analytics.
Clicking the green "YES" button saves the user setting and starts allowing tracking calls to be sent to google analytics.
We are tracking the following things:
react-router
)Users can opt in or out of this feature by going to Reactotron -> Settings (which opens the settings json file in their default editor.
Editing this file to have the
analyticsOptOut
betrue
orfalse
and then restarting the app will turn this setting on and off.Things that we need to do before this can move out of draft status:
GA4_KEY
variable.