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

Fix Google Analytics tracking #81

Merged
merged 3 commits into from
Sep 26, 2017
Merged

Conversation

tiffon
Copy link
Member

@tiffon tiffon commented Sep 22, 2017

Fix #80.

Created an HOC that checks the state of the router and tracks a page-view whenever the URL or query string changes. This is necessary because react-router v4 did away with the onUpdate lifecycle event.

const defaultHistory = createHistory();

const TrackedSearchPage = trackedComponentEnahncer(ConnectedSearchTracePage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why break this up? If we do it at the top level container, we don't have to add it to each page.

If our concern is that the Page only mounts once, I would hook into React router page change event and do it there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch my comment, didn't realize we were on react router v4.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can do something similar to this helper library
https://github.com/pshrmn/rrc/blob/master/docs/OnUpdate.md

It would be nice if we could listen to all changes on the location without explicitly putting it on each page component. That way, when a new page is added, it will automatically start tracking it without having to use this HOC. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, could possibly connect it to the store and use state.location instead... I'll look into that.

Copy link
Member Author

@tiffon tiffon Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saminzadeh Moved tracking into Page.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 56.1% when pulling a9eb89b on issue-80-fix-google-analytics into 5162309 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 56.055% when pulling 8f0b60a on issue-80-fix-google-analytics into 5162309 on master.


componentDidMount() {
const { pathname, search } = this.props.location;
trackPageView(pathname, search);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I think this is more robust.

One comment, given that the GA key will come through the HTTP Config service (async), we need to make sure the key is initialized beforehand so we don't miss any page view events

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I might make a separate ticket for that. Don't want to hold up fixing GA tracking or getting the menu working.

@tiffon tiffon merged commit 5139c7f into master Sep 26, 2017
@yurishkuro yurishkuro deleted the issue-80-fix-google-analytics branch January 29, 2020 15:05
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Fix Google Analytics tracking

* Google Analytics tracking moved to a higher level component

* Misc css tweak

Signed-off-by: vvvprabhakar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants