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.

Sign up for GitHub

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

Jump to bottom

Add Umeng+ analytics config #13099

Merged
merged 4 commits into from Feb 16, 2018
Merged

Add Umeng+ analytics config #13099

merged 4 commits into from Feb 16, 2018

Conversation

gzgogo
Copy link
Contributor

@gzgogo gzgogo commented Jan 29, 2018

@gzgogo
Copy link
Contributor Author

gzgogo commented Jan 30, 2018 

DESCRIBE => Rendering of amp-img
  IT => should respect media queries
    ✗ expected 'i-amphtml-element i-amphtml-layout-responsive i-amphtml-layout-size-defined i-amphtml-layout' to match /i-amphtml-hidden-by-media-query/
    AssertionError@base/node_modules/chai/chai.js?40e7aa72e9665366bfd82579520de4fb0754dfae:9320:22
    assert@base/node_modules/chai/chai.js?40e7aa72e9665366bfd82579520de4fb0754dfae:239:31
    assertMatch@base/node_modules/chai/chai.js?40e7aa72e9665366bfd82579520de4fb0754dfae:2452:16
    methodWrapper@base/node_modules/chai/chai.js?40e7aa72e9665366bfd82579520de4fb0754dfae:7709:30
    [native code]
    absolute/tmp/d04a538df3a7c0d8932460a95c26343d.browserify?35d55fc681f2bc623165b165502a6f7061f59c12:82109:47
    run@absolute/tmp/d04a538df3a7c0d8932460a95c26343d.browserify?35d55fc681f2bc623165b165502a6f7061f59c12:14520:29
    absolute/tmp/d04a538df3a7c0d8932460a95c26343d.browserify?35d55fc681f2bc623165b165502a6f7061f59c12:14533:33
    flush@absolute/tmp/d04a538df3a7c0d8932460a95c26343d.browserify?35d55fc681f2bc623165b165502a6f7061f59c12:11649:11

I had not change the amp-img component which make the CI failed.

@gzgogo
Copy link
Contributor Author

gzgogo commented Feb 5, 2018

@zhouyx thanks!

zhouyx
zhouyx suggested changes Feb 6, 2018
View reviewed changes
extensions/amp-analytics/0.1/vendors.js Outdated
'event': '${base}&_ename=${eventName}&_eprops=${eventProps}',
},
'transport': {
'beacon': false,
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm that your server only support transport type image. We recommend using beacon, and AMP will handle switching to other transport method when browser doesn't support beacon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not support beacon now and will support it later.

extensions/amp-analytics/0.1/vendors.js
'eventName': '',
'eventProps': '',
},
'requests': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm that you don't provide default trigger in your vendor config? even pageview event?

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 I confirm，Because if there are multiple amp-analytics components with the same type on a page, the default pageview will be trigger multiple times. So we removed it. And let the users decide whether to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good to me as long as you document the usage clearly on your site : )

@ghost ghost mentioned this pull request Feb 8, 2018
Merged
zhouyx
zhouyx suggested changes Feb 8, 2018
View reviewed changes
Copy link
Contributor

@zhouyx zhouyx 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, but please fix the example before we merge the PR. Thanks

examples/analytics-vendors.amp.html Outdated
<script type="application/json">
{
"vars": {
"siteid": "YOUR_SITEID"
Copy link
Contributor

Choose a reason for hiding this comment

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

I was testing locally with your config here. I got error log.js:162 [AmpAnalytics umenganalytics] Analytics config could not be parsed. Is it in a valid JSON format? SyntaxError: Unexpected token � in JSON at position 47
I check it's because of _ here. I guess the _ here is not a standard underscore. Please fix it.

This is also why I suggest adding default trigger to your vendor config. Because it's just too common to misconfigure the json object 😢.

Please fix the example here.

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 fixed it, and added the defaultPageview trigger.

@zhouyx
Copy link
Contributor

zhouyx commented Feb 13, 2018

Please rebase to fix the integration test, so that I can go ahead and merge this. Thanks.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@zhouyx
Copy link
Contributor

zhouyx commented Feb 14, 2018

@gzgogo Please try

git checkout master
git pull
git checkout <branch name>
git rebase master

@googlebot
Copy link

CLAs look good, thanks!

@zhouyx zhouyx merged commit 885d81a into ampproject:master Feb 16, 2018
clawr pushed a commit to clawr/amphtml that referenced this pull request Feb 20, 2018
@gzgogo @clawr
Add Umeng+ analytics config (ampproject#13099)
f842054 
* repair master

* ut

* update ut

* update ut
protonate pushed a commit to protonate/amphtml that referenced this pull request Feb 26, 2018
@gzgogo
Add Umeng+ analytics config (ampproject#13099)
be536ef 
* repair master

* ut

* update ut

* update ut
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
@gzgogo @RanAbram
Add Umeng+ analytics config (ampproject#13099)
1354afc 
* repair master

* ut

* update ut

* update ut
protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018
@gzgogo
Add Umeng+ analytics config (ampproject#13099)
7aa70a3 
* repair master

* ut

* update ut

* update ut
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhouyx zhouyx zhouyx approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gzgogo @zhouyx @googlebot