Skip to content
This repository was archived by the owner on Dec 11, 2019. It is now read-only.

Conversation

@duellsy
Copy link
Contributor

@duellsy duellsy commented Jan 4, 2018

Removes confusion for customers who aren't immediately calling page(), and wondering why the elevio script hasn't loaded yet.

duellsy added 16 commits March 9, 2017 10:32
This helps to future proof for companies using the latest version of the embeddable
…ytics.js-integration-elevio

* 'master' of https://github.com/segment-integrations/analytics.js-integration-elevio:
  Bump a.js-int-tester version to ^3.1.0 (segment-integrations#7)
  Pin karma, karma-mocha dev dependencies (segment-integrations#6)
  Release 2.1.0

# Conflicts:
#	HISTORY.md
#	package.json
Had double quotes instead of single. Sorry.
* analytics-master:
  Release 2.2.1
…ration-elevio

* 'master' of https://github.com/elevio/analytics.js-integration-elevio:
  Reversal of setUser function check logic
  Changing setUser function exists test to be more explicit
  Linting issue
  Re-instating package.json updates that a merge missed
  Updated version to 2.2.0
  Added support for using new setUser method
  Release 2.1.0
  Added support for using plan as group
Updated test to remove the call to assumesPageView
@codecov-io
Copy link

codecov-io commented Jan 4, 2018

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #12   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          34     34           
=====================================
  Hits           34     34
Impacted Files Coverage Δ
lib/index.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6155cfd...fd33d39. Read the comment docs.

@duellsy
Copy link
Contributor Author

duellsy commented Jan 4, 2018

Just looping in @hankim813 since you accepted the last PR for this project, hoping you can do the same here :)

@hankim813
Copy link
Contributor

hey @duellsy thanks for the PR — I moved out of eng recently but @jlee9595 should be able to help here.

@jlee9595
Copy link

jlee9595 commented Jan 5, 2018

Hi @duellsy ! I believe that all that assumesPageView does is cause the first page call to be ignored, and has no effect on when the script is loaded. Would you mind providing me with a valid Elevio account id I can use to test this knowledge myself on the integration? If I end up being wrong about that then this PR looks great to me.

@duellsy
Copy link
Contributor Author

duellsy commented Jan 5, 2018 via email

@jlee9595
Copy link

jlee9595 commented Jan 5, 2018

Hey @duellsy I'm currently looking into this and will get back to you later today

@jlee9595
Copy link

jlee9595 commented Jan 5, 2018

@duellsy One question I have is whether the Elevio script does some kind of page when it loads. The purpose of assumesPageView is to prevent integrations with scripts that do a page when they load from inaccurately recording two pages, one from the script loading and one from the first Segment page call, by causing the first Segment page to be dropped. If the Elevio script doesn't page when it loads then I'd say that you should definitely go ahead and get rid of assumesPageView but if it does page on load, then I think we should find an alternate solution to your problem here.

@jlee9595
Copy link

jlee9595 commented Jan 5, 2018

@duellsy Elevio is using version 2 of analytics.js-integration and doesn't have the following update to assumesPageView which came in 3.2: segmentio/analytics.js-integration@7597a6f

My final recommendation is to first update the analytics.js-integration dependency, but if Elevio doesn't automatically page on load then you should remove assumesPageView as well for the reasons described above.

@duellsy
Copy link
Contributor Author

duellsy commented Jan 5, 2018

@jlee9595 ooh, ok.

Am I correct in thinking if we upgrade analytics.js-integration to 3.2 this will allow our script to load immediately rather than waiting for the page call to be made by the hosting site?

Essentially, in the example here, we'd like the elevio script to be loaded immediately, rather than waiting for the page call to be made (which you can simulate by clicking the button in the middle of the screen)

https://jsfiddle.net/duellsy/xcfzygcp/

Is this something that is possible, can you foresee issues with this?

@jlee9595
Copy link

jlee9595 commented Jan 5, 2018

@duellsy That's correct! Upgrading to 3.2 should give you what you want.

@duellsy
Copy link
Contributor Author

duellsy commented Jan 6, 2018

Fantastic, thanks @jlee9595, just updated to 3.2 and pushed

🎉

@jlee9595 jlee9595 merged commit 9262cd7 into segment-integrations:master Jan 8, 2018
duellsy added a commit to dixahq/analytics.js-integration-elevio that referenced this pull request Jul 7, 2018
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