Skip to content
This repository has been archived by the owner on Oct 23, 2019. It is now read-only.

Add ability to support google manager tracker #10

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

jtapia
Copy link

@jtapia jtapia commented Mar 17, 2018

What it does?

  • Add ability to specify what kind of tracker do you want to use on the admin
  • Add script for google manager
  • Remove unnecessary columns
  • Add tracker_type to spree_trackers DB
  • Refactor current method
  • Add and fix test suite

Preview

screencapture-localhost-4000-admin-stores-2-edit-2018-03-16-14_32_29

screencapture-localhost-4000-admin-trackers-2018-03-16-15_00_24

screencapture-localhost-4000-admin-trackers-new-2018-03-16-14_49_55

@jtapia jtapia force-pushed the master branch 2 times, most recently from dd43aee to f6ce890 Compare May 1, 2018 21:19
@fastjames
Copy link

@jtapia Would you mind rebasing this? We are swinging back to some extensions with open PRs, and we would like to merge this one (this work is relevant to our interests too).

@jtapia jtapia force-pushed the master branch 2 times, most recently from 118788a to cb0f5a6 Compare November 20, 2018 17:29
Copy link

@fastjames fastjames left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

Thanks @jtapia! This looks okay to me, other than the odd indentation.

Do you mind taking a look @kennyadsl?

app/views/spree/shared/_google_analytics.html.erb Outdated Show resolved Hide resolved
@jtapia jtapia force-pushed the master branch 2 times, most recently from d08da34 to c3d3952 Compare November 20, 2018 18:17
@jtapia jtapia force-pushed the master branch 2 times, most recently from 2ce73f9 to e03efe0 Compare November 20, 2018 20:51
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Having two active trackers is a requirement? If not I think we should try to don’t change the .current API and set the type of existing trackers via the default value of the migration that add the type column. What do you think?

@jtapia
Copy link
Author

jtapia commented Nov 20, 2018

@kennyadsl yes, that's the main idea, to have multiple tracking services at the same time, I made this 3376cb2 to avoid iteration on the views and to avoid changes on self.current method, what do you think?

@jtapia jtapia force-pushed the master branch 3 times, most recently from dc9f137 to 8d1ecb0 Compare November 21, 2018 00:36
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Left some comments. Also, did we have something that auto-mark old trackers as non-active when you activate a new one? Do you think we could need something similar? Maybe into another PR?

app/models/spree/tracker.rb Outdated Show resolved Hide resolved
app/models/spree/tracker.rb Show resolved Hide resolved
app/models/spree/tracker.rb Outdated Show resolved Hide resolved
app/models/spree/tracker.rb Outdated Show resolved Hide resolved
@jtapia jtapia force-pushed the master branch 10 times, most recently from 4256f35 to 28ff366 Compare November 24, 2018 00:14
@jtapia
Copy link
Author

jtapia commented Nov 26, 2018

@kennyadsl once this is merge, I'll bring these changes jtapia@488dad5 to include facebook pixel

@kennyadsl
Copy link
Member

There is one thing I can't get: this PR is adding the main script for Tag Manager which, as far as I know, only defines a DataLayer you should push events on but right now no events has been pushed so I'm not sure what it's tracking. Do we at least need to support the pageView event? How does it work tracking other actions (like purchase) for solidus_trackers users?

@jtapia jtapia force-pushed the master branch 2 times, most recently from 1399a63 to 50068a3 Compare November 26, 2018 20:48
@jtapia
Copy link
Author

jtapia commented Nov 26, 2018

@kennyadsl right, here is the change for that 907fc64#diff-ef80dbd5bb074228fe38463e26a993f6R17

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants