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

remove mysql page view tracking #2750

Merged
merged 3 commits into from
Apr 21, 2018
Merged

Conversation

bnchdrff
Copy link
Contributor

closes steemit/steempunks#321

remove page_view api resource
do not try to display a page view count
only record page views to overseer

closes steemit/steempunks#321

remove page_view api resource
do not try to display a page view count
only record page views to overseer
@bnchdrff bnchdrff requested review from sneak and gl2748 April 20, 2018 20:03
if (last_page_promise && page === last_page) return last_page_promise;
if (window.ga) {
// virtual pageview
window.ga('set', 'page', page);
window.ga('send', 'pageview');
}
api.call('overseer.pageview', { page, referer: ref, account }, error => {
// if (error) console.warn('overseer error', error, error.data);
last_page_promise = api.callAsync('overseer.pageview', {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be wrapped in a process.env.BROWSER check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good point :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

err actually oddly enough we're fixing a previous bug here; looks like we have been double-recording ssr pageviews to overseer !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roadscape "fixed"

className="PageViewsCounter"
title={tt('g.views', { count: views }) + suffix}
>
<Icon name="eye" /> {views.toLocaleString()}
Copy link
Contributor

Choose a reason for hiding this comment

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

if we might reuse this at some point we should keep the render function

Copy link
Contributor

Choose a reason for hiding this comment

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

just because classnames, css, icon are all known working

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'd rather not include it as commented code; we can find the old markup here in git

@roadscape
Copy link
Contributor

Code looks good, let's merge after dev test

Copy link
Contributor

@gl2748 gl2748 left a comment

Choose a reason for hiding this comment

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

LGTM.

@gl2748
Copy link
Contributor

gl2748 commented Apr 21, 2018

Before

screenshot from 2018-04-20 21-17-28

After

screenshot from 2018-04-20 21-17-42

@gl2748 gl2748 merged commit 5257b62 into master Apr 21, 2018
@bnchdrff bnchdrff deleted the steempunks321-mysql-pageview-removal branch April 22, 2018 02:05
@skzap
Copy link

skzap commented Apr 22, 2018

Removing views is a nice change. This was becoming a very wrong number with the emergence of new apps on the steem blockchain.

smoke-indica added a commit to smoke-indica/webapp that referenced this pull request Aug 5, 2020
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.

4 participants