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
#759 google ecommerce #760
Conversation
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.
minors: naming, api data types, tests proposition
front/js/shared/tracking.es6
Outdated
@@ -78,6 +67,44 @@ | |||
}); | |||
} | |||
|
|||
// @todo #759:30m Move PublishedGATracker to a separate file. | |||
|
|||
class PublishedGATracker { |
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.
provide doc plz why it's published, but not common GATracker
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.
publish is not the most good name for classes purpose. LimitedGATracher
or SafeGATracker
might be more good. Up to you
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 believe LoadedGATracker
would be the best name
front/js/shared/tracking.es6
Outdated
ga('require', 'ecommerce'); // Ignore ESLintBear (block-scoped-var) | ||
const tracker = new GATracker(ga, 'ecommerce'); // Ignore ESLintBear (block-scoped-var) | ||
|
||
tracker.purchase(productsData, txData); |
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.
hope data is consistent with gAnalytics api docs. Looks too simple and suspicious =)
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.
Data were consistent before, so what could be wrong?
front/js/shared/tracking.es6
Outdated
this.published = false; | ||
} | ||
|
||
purchase(productsData, txData) { |
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.
maybe it's possible to add tests for ecommerce behaviour.
We are already testing Ya.Metrika at SE: shopelectro.tests.tests_selenium.YandexMetrika
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 will create todo for it
Closes #759