-
Notifications
You must be signed in to change notification settings - Fork 59
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: integrate analytics #244
Conversation
Codecov Report
@@ Coverage Diff @@
## master #244 +/- ##
=========================================
Coverage ? 63.08%
=========================================
Files ? 370
Lines ? 8115
Branches ? 1435
=========================================
Hits ? 5119
Misses ? 2996
Partials ? 0 Continue to review full report at Codecov.
|
a3f9940
to
a2f4ee4
Compare
env.js
Outdated
@@ -27,6 +27,10 @@ const STATUS_URL = process.env.STATUS_URL; | |||
// This should only be used when Admin is also in unsecured mode. | |||
const DISABLE_AUTH = process.env.DISABLE_AUTH; | |||
|
|||
// Configure Google Analytics | |||
const DISABLE_GA = process.env.DISABLE_GA; | |||
const GA_TRACKING_ID = process.env.GA_TRACKING_ID; |
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.
We want this to default to G-0QW4DJWJ20
. so process.env.GA_TRACKING_ID || 'G-0QW4DJWJ20
src/client.tsx
Outdated
@@ -11,6 +12,12 @@ const render = (Component: React.FC) => { | |||
const initializeApp = () => { | |||
const App = require('./components/App/App').App; | |||
|
|||
const { DISABLE_GA, GA_TRACKING_ID } = env; | |||
|
|||
if (!DISABLE_GA) { |
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 doesn't seem to work as intended. ENV values of "false" or false
both don't seem to fire this.
a2f4ee4
to
51dd533
Compare
Signed-off-by: Pianist038801 <[email protected]>
51dd533
to
42fc6c6
Compare
Signed-off-by: Pianist038801 <[email protected]>
README.rst
Outdated
|
||
This application makes use of the `react-ga4 <https://github.com/PriceRunner/react-ga4>`_ | ||
libary to include Google Analytics tracking code in a website or app. For all the environments, it is configured using ENABLE_GA environment variable. | ||
By default, it's enabled like this: ``ENABLED=true``. If you want to disable it, just remove the value. (ex. ``ENABLE_GA='``). |
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 says ENABLED=true
but it should be ENABLE_GA =true
, right?
Signed-off-by: Pianist038801 <[email protected]>
src/client.tsx
Outdated
@@ -11,6 +12,12 @@ const render = (Component: React.FC) => { | |||
const initializeApp = () => { | |||
const App = require('./components/App/App').App; | |||
|
|||
const { ENABLE_GA, GA_TRACKING_ID } = env; | |||
|
|||
if (ENABLE_GA) { |
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.
All env vars come back as strings so this will always be true if the var exists. That means you'll need to do an explicit check here for either !="false" || !="0"
.
Signed-off-by: Pianist038801 <[email protected]>
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.
lgtm
# [0.35.0](http://github.com/lyft/flyteconsole/compare/v0.34.0...v0.35.0) (2021-12-15) ### Features * integrate analytics ([#244](http://github.com/lyft/flyteconsole/issues/244)) ([2726b07](http://github.com/lyft/flyteconsole/commit/2726b07a012a8a12b35752fbe714e1a148f630f7))
🎉 This PR is included in version 0.35.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
* feat: integrate analytics Signed-off-by: Pianist038801 <[email protected]> * fix: remove DISABLE_AUTH Signed-off-by: Pianist038801 <[email protected]> * fix: typo in the README Signed-off-by: Pianist038801 <[email protected]> * fix: analytics configuration Signed-off-by: Pianist038801 <[email protected]> Co-authored-by: Pianist038801 <[email protected]> Signed-off-by: csirius <[email protected]>
# [0.35.0](http://github.com/lyft/flyteconsole/compare/v0.34.0...v0.35.0) (2021-12-15) ### Features * integrate analytics ([#244](http://github.com/lyft/flyteconsole/issues/244)) ([2726b07](http://github.com/lyft/flyteconsole/commit/2726b07a012a8a12b35752fbe714e1a148f630f7)) Signed-off-by: csirius <[email protected]>
Signed-off-by: Pianist038801 [email protected]
This integrates Google Analytics into the console application.
Are all requirements met?
Tracking Issue
fixes flyteorg/flyte#1442
Follow-up issue
NA