Skip to content

Bidtellect amp implementation…#9518

Merged
zhouyx merged 7 commits intoampproject:masterfrom
bidtellect:bidtellect
May 31, 2017
Merged

Bidtellect amp implementation…#9518
zhouyx merged 7 commits intoampproject:masterfrom
bidtellect:bidtellect

Conversation

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor

@edaquinta-bidtellect edaquinta-bidtellect commented May 24, 2017

No description provided.

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

CLA have been signed 20 days ago. Company name is Bidtellect.

@jridgewell
Copy link
Copy Markdown
Contributor

Looks like you'll need to rebase off current master.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

??? This is brand new branch. I just branched from master and made the changes. Only one commit.

Copy link
Copy Markdown
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.

@edaquinta-bidtellect Thanks for the Pull Request again.
Few things: I am able to see your company name in our Pending Corporate CLA list. So I expect the CLA to be good to go soon.
Regarding the conflict, did you rebase to the lastest version before making changes? We refactored our ads example page last week to make it cleaner. Please try rebase and fix conflict.
Last I see you put renderStartImplemented: true, but when I test locally I am not able to receive render-start message, please double check and fix it.
Otherwise the PR looks good! Please let me know if you have any question.

* @param {!Object} data
*/
export function bidtellect(global, data) {
const requiredParams = ['t','pid','sid'];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: space after ,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

??? Didn't get this comment...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

const requiredParams = ['t', 'pid', 'sid'];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

},

bidtellect: {
renderStartImplemented: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see you implement render-start, but when I test locally I am never able to receive it? Could you double check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will look into this.

@zhouyx zhouyx self-assigned this May 26, 2017
@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

I made this changes in a new branch from master. I delete old one, create a complete new branch from master and made the changes. I shouldn't have any conflicts. I don't see any conflicts.

@zhouyx
Copy link
Copy Markdown
Contributor

zhouyx commented May 26, 2017

I guess your master has conflict by the time you create new branch from it.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

This start becoming annoying for me. I can't see conflicts or any new commit. I use SourceTree on Mac, merge master into my brach and push. No conflicts. Pull latest from master, no new commits.

@zhouyx
Copy link
Copy Markdown
Contributor

zhouyx commented May 26, 2017

@edaquinta-bidtellect Sorry I've never used SourceTree, so can't help much. Please refer to our getting started doc for a general guidline.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

git rebase master
Current branch bidtellect is up to date.

@jridgewell
Copy link
Copy Markdown
Contributor

You'll need to rebase off of the upstream master (given you named the ampproject/amphtml repo upstream):

git remote add upstream https://github.com/ampproject/amphtml.git
git fetch upstream
git checkout bidtellect
git pull --rebase upstream master
git push -f origin bidtellect

…idtellect

* 'master' of https://github.com/ampproject/amphtml: (247 commits)
  Move custom middleware before Karma's builtin middleware. (#9561)
  admanmedia ad network type added (#8052)
  Animations: width, height and rand functions (#9539)
  ES6xize node server code. (#9578)
  amp-bind: Reference I/O codelab in docs (#9570)
  Fix most amp-bind integration tests (#9577)
  skip bind tests to unbreak master (#9572)
  Animations: minor spec references (#9554)
  Order element-specific actions lexicographically (#9569)
  Add comment to applylayout method for SSR (#9551)
  Wait for document ready before search for element in #getElement (#9486)
  add missing glob dep (#9564)
  Report when XHR are issued before viewer is visible (#9350)
  Fix gitignore (#9510)
  Increase amp-bind integration test timeout (#9558)
  add chalk dev dep (#9555)
  Filter out script links from .md files during check-links and make failures PR blocking (#9552)
  Fix render-delaying-extension false error report (#9382)
  allow all z-index to be overridable (#9538)
  update minimum version of node from 4.0.0 to 6.10.3 (#9537)
  ...

# Conflicts:
#	examples/ads.amp.html
@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

Let me know now... Thanks @jridgewell

@jridgewell
Copy link
Copy Markdown
Contributor

Merge conflicts are gone, now just waiting for Travis and the CLA Bot to OK.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

Travis failed. Not sure If I need to do something.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

https://cdn.ampproject.org is failing

@jridgewell
Copy link
Copy Markdown
Contributor

The failure is unrelated. We'll fix this on master, but you'll need to rebase again after it's merged. Sorry.

/cc @rsimha-amp

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

God bless me... lol

@jridgewell
Copy link
Copy Markdown
Contributor

Worse case, we can force merge even with this failure.

…idtellect

* 'master' of https://github.com/ampproject/amphtml:
  remove instruction to add .max (#9592)
  turn on hidden-v3 (#9589)
  Allow for functional links in amp-accordion headers (#9335)
  Allow option attribute for children of amp-selector in templates (#9585)
  Improve CORS documentation (#9583)
  Should show amp-app-banner in non-embedded firefox ios (#9573)
  Revert "update minimum version of node from 4.0.0 to 6.10.3 (#9537)" (#9560)
@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented May 26, 2017

Sorry about that. I've whitelisted the false negative failure in #9596

@jridgewell
Copy link
Copy Markdown
Contributor

Awesome, do you mind rebasing one more time? Sorry about all the trouble.

…idtellect

* 'master' of https://github.com/ampproject/amphtml:
  amp-bind: Handle attributes on input elements that sometimes only specify initial values (#9584)
  Tidy up doc for amp-3q-player for consistency w/ other components (#9606)
  Tidy up doc for amp-timeago - consistency w/ other components (#9605)
  amp-ima-video: Markdown README for project (#9336)
  Add missing actions to "all elements" table (#9562)
  Fixed typo in amp-img closing tag (#9604)
  Add unit test for ad concurrent load. (#9599)
  Enable _ga cookie for googleanalytics, doubleclick and adsense. (#9586)
  Whitelist check-links.js from cdn URL presubmit check (#9600)
  Whitelist expected non-working links for link checker (#9596)
  Allow tests to access local version of web worker. (#9475)
  Origin trials (#8944)
  XHR#fetchText now returns Response (#9567)
@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

Done! Still no CLA though.

@jridgewell
Copy link
Copy Markdown
Contributor

Have you confirmed the CLA on your side? Maybe you're missing some info? If not, I'll escalate with the CLA reviewers.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

I have no idea about CLA. I only know we signed a month ago and I'm sure I'm in the google group as an owner. But other than that I don't know nothing else. Waiting all this time without any feedback.

@jridgewell
Copy link
Copy Markdown
Contributor

Alright, I've escalated. They're generally pretty responsive (a few hours at most), so we should be able to track down the issue shortly.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

Thanks!

@jridgewell
Copy link
Copy Markdown
Contributor

Seems Kim Bamback needs to sign the CLA. An email was sent to her with a online form. They just resent.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

She signed already. Today in the morning (ET). Any updates?

@jridgewell
Copy link
Copy Markdown
Contributor

None on my end, should't be long though.

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

CLA is done! Anything else?

@jridgewell
Copy link
Copy Markdown
Contributor

Yay! @zhouyx needs to approve.

Copy link
Copy Markdown
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.

LGTM. Please confirm that you no longer implement renderStart in this PR, so we can go ahead and merge.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

You can merge it! I did not implemented renderStart.

@zhouyx zhouyx merged commit dd7e5de into ampproject:master May 31, 2017
@zhouyx
Copy link
Copy Markdown
Contributor

zhouyx commented May 31, 2017

Thanks for the Pull Request. Merged.

@edaquinta-bidtellect
Copy link
Copy Markdown
Contributor Author

Thanks @jridgewell and @zhouyx. Sorry for the merge issues ;)

@edaquinta-bidtellect edaquinta-bidtellect deleted the bidtellect branch May 31, 2017 19:57
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jun 6, 2017
* Bidtellect amp implementation…

* removing renderStartImplemented…

* spaces?

* adding example
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jun 6, 2017
* Bidtellect amp implementation… (ampproject#9518)

* Bidtellect amp implementation…

* removing renderStartImplemented…

* spaces?

* adding example

* Fix test-amp-ad-network-doubleclick-impl (ampproject#9645)

* Optimizations to speed up Travis PR builds (ampproject#9626)

Travis builds have become slow during busy periods of the day due to various reasons. This PR does the following:

Reorganizes build tasks across fewer build shards
Reduces the number of VMs used by PR and master builds to 2
No longer does a full gulp dist for unit tests (Possible to do after ampproject#9404)
Reduces the total CPU time used for PR builds to < 20 mins
Fixes ampproject#9500
Fixes ampproject#9387

* Animations: full on-action API (ampproject#9641)

* Animations: full on-action API

* review fixes

* Fix Validator tests on Travis, which now uses Node v4 (ampproject#9654)

* Amp-imgur : Implement imgur embed (ampproject#9405) (ampproject#9434)

* Amp-imgur : Implement imgur embed (ampproject#9405)

* amp-imgur test code modified

* amp-imgur validation code changed

* fix amp-imgur lint check

* Remove unused Layour variables

* Test code modified

* Ingegration amp-imgur extension

* [amp-imgur] add resize logic

* Remove 3p iframes and add resize message listener

* Fix some code with reviews
* change some syntax
* change validation rules
* add find event source

* [amp-imgur] remove unnecessary code and add object check

* remove spec_name from validation code

* Remove unnecessary code and add owners file

* Change required tag name  to  in validation file

* data-imgurid -> data-imgur-id

* Add whitelist for amp-imgur

* remove unused AmpImgur import in test file

* remove unused preconnect callback

* Update amp-cors-requests.md (ampproject#9636)

Added updates and corrections per Dima's feedback.

* Expose login url building to amp access service (ampproject#9300)

* Animations: support style keyframes (ampproject#9647)

* Animations: support style keyframes

* docs

* review fixes

* lints

* Animations: subtargets format (ampproject#9655)

* Animations: subtargets format

* fix docs

* Fix extern for integration test (ampproject#9657)

* Remove whitelisted link for PR 9434 (ampproject#9656)

* Add callouts for CORS + cleanup (ampproject#9591)

* Add callouts for CORS + cleanup

* Update amp-access.md

* Doubleclick Fast Fetch: Send default safeframe version on ad request (ampproject#9613)

* Send default safeframe version on ad request

* fix test failure

* amp-ima-video: Fixes some undefined variables in compiled extension (ampproject#9617)

* Add experimental input-debounced to the actions and event doc (ampproject#9650)

* Copy and rename compiled script directly for deprecated version (ampproject#9587)

* Remove A4A Dependency on ads/_config.js (ampproject#9462)

* Added doubleclick specific config

* Modified other networks

* Update MockA4AImpl

* Remove unneeded config lines

* Add cid, remove renderStartImplemented

* Fix lint complaint

* Changed from mandatory config to overrrideable method

* Addressed feedback, modified getAdCid

* Addressed additional feedback

* Responded to feedback

* Removed unnneeded guard against null value

* Changed signature of function

* Fixed lint issues

* addressed comments

* Addressed comments, fixed broken test

* Change signature / update tests

* Fix wrong contents and white spaces in amp-imgur.md (ampproject#9658)

* Performance: separate ini-load from first visible ini-load (ampproject#9665)

* amp-bind: Fix more integration tests (ampproject#9598)

* more bind test fixes

* include object.assign polyfill

* move polyfills to separate file

* replace use of map() from bind-validator with ownProperty()

* fix types and test

* Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow) (ampproject#9668)

* removing overflow touch on no-scroll for ios

* Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow)

* make sure css is compiled before entry points (ampproject#9643)

* update npm package version (ampproject#9671)

* A4A: expose visibilityCsi (ampproject#9667)

* A4A: expose visibilityCsi

* tests

* amp-bind: Fix embedding in FIF (ampproject#9541)

* move element service changes from ampproject#9447

* store element service ids in extension struct

* add unit test, fix other tests

* Fix null value binding (ampproject#9674)

* Add amp-form's submit action to the docs (ampproject#9675)

* Write cookie for ad-cid. (ampproject#9594)

* Write cookie for ad-cid.

* fix tests

* amp-access-laterpay fixes (ampproject#9633)

* Add support for LaterPay subscriptions

* Fix canonical link on laterpay example

* Tweak default styling

* Add an example locale message for the header

* Fix indentation of JsDoc (ampproject#9677)

It should be indented at the same level as the method it's documenting.

* Add example to amp iframe docs (ampproject#9660)

* Change image to static link hosted on ampproject.org

* Update amp-iframe doc w example + gen cleanup

* Adds SFG experiment branches to doubleclick-a4a-config.js. (ampproject#9662)

* Added experiment branches corresponding to exp=a4a:(5|6).

* Removed test file.

* Removed changes to yarn.lock + minor fixes.

* Fixed adsense-a4a-config.js.

* Fixed return statement.

* Removed reference to style tag for opacity (ampproject#9634)

As style tag for `opacity` was replaced with `amp-boilerplate (back in this [commit](ampproject@0a056ca), updated the text to reflect those changes.

* De-flake amp-bind integration tests (ampproject#9683)

* split bind fixtures into one file per extension

* fix width/height binding bug, avoid race condition w/ dynamic containers

* actually, just skip the tests affected by race condition for now

* Remove timer calls to prevent future flakiness (ampproject#9478)

* Remove timer calls to prevent future flakiness

* Revert the line order change

* Poll instead of using stub callback constructor

* Add jsdoc and rename variables

* amp-animation: polyfill partial keyframes from CSS (ampproject#9689)

* Animations: whitelist offset distance and regroup condition types (ampproject#9688)

* Use Docker containers in Travis (ampproject#9666)

* clean up web-worker experiment (ampproject#9706)

* Load examiner.js when #development=2 (ampproject#9680)

* Load examiner.js when #development=1

* address comments

* Use development=2

* Fix presubmit

* Implementation of amp-ad-exit (ampproject#9390)

* Implementation of the amp-ad-exit component for managing ad navigation (ampproject#8515).

Changes from the I2I:
- Only RANDOM, CLICK_X, and CLICK_Y variables can be used, alongside custom vars.
- It doesn't work well with <a> tags - the tap action doesn't trigger on middle clicks. Recommendation is to use other elements.
- ClickLocationFilter for border click protection is not yet implemented. It will be added in a future change.

The component does some lightweight validation of the JSON config when it's
built. This may be overkill.

Tested:
gulp test --files 'extensions/amp-ad-exit/0.1/test/*'
gulp check-types

* Update JSDoc for some functions. Fix lint issue (ignore unused parameters in an interface method).

* sinon.spy -> sandbox.spy

* Add amp-ad-exit to the list of allowed extensions in A4A pages and fix validation rules.

Address PR: add experiment flag; use camelCase for config properties

address some comments

Instantiate a new Filter for each spec.

* Instantiate a new Filter for each spec.

* remove duplicate doctype tag in example

* Make ActionEventDef public for type annotations.

* Add filter factory.

* fix event var

* update type annotation for Filter.filter

* address PR comments

* Move filter config validation to the ctor. Remove Filter.buildCallback().

* camelCase in documentation examples

* Introducing "it.yield", a convenient way to test promise code (ampproject#9601)

* Introduce yield.

* Address comments.

* fix done state error handling

* Document amp-access as a special target (ampproject#9568)

`amp-access` is a special target that's not mentioned in this document.
It's special because you can't give an arbitrary ID to it, i.e., you
can't just change the id of the `amp-access` script tag to `amp-access2`
and expect `on="tap:amp-access2.login"` to work. Given that the
"actions" for `amp-access` is a bit dynamic depending on the structure
of the `amp-access` JSON, instead of listing out the actions in a
tabular form, a reference to the `amp-access` documentation is added.

* Run presubmit tests soon after building (ampproject#9716)

* Revert "Use Docker containers in Travis (ampproject#9666)" (ampproject#9717)

This reverts commit 98ecb9b.

It turns out that while using Docker containers instead of VMs on Travis reduces VM startup time, it has significantly increased build and test time, increasing the overall PR check round trip from < 20 mins to > 30 mins.

Compare https://travis-ci.org/ampproject/amphtml/jobs/238464404 (Docker) against https://travis-ci.org/ampproject/amphtml/jobs/238462066 (VM).

Fixes ampproject#9651

* Validator Rollup (ampproject#9719)

* Add validator tests for amp-imgur.

* Disallow amp-embed as child of amp-app-banner.

* Minor cleanup, Make sure our set numbers are consistent in javascript.

* amp-state: Allow both `src` and script child (ampproject#9721)

* adapt code from ampproject#8897

* also_requires_attr should be in trigger

* Remove experiment for input-debounced. Update docs. (ampproject#9724)

* Validator Rollup (ampproject#9727)

* Minor cleanup of amp-ad-exit

* Disallow amp-ad/amp-embed as children of amp ad containers when data-multi-size is present.

* Revision bump

* Enabling dynamic queryparam addition to anchour links  (ampproject#9684)

* Enabling queryparam addition to anchour links

* Code review changes

* Additional cache urls (ampproject#9733)

* Significantly speed up gulp build --css-only (ampproject#9726)

Prior to this PR, gulp build --css-only would take ~ 1 minute to run, and end up building a bunch of js files in addition to compiling css.

After this PR, gulp build --css-only takes ~ 0.5 seconds to run (a 100x speed up), and the only output files in build/ after running it are .css, and .css.js files.

Resulting speeds on my workstation:
gulp build --css-only: ~500ms
gulp build: ~1m 45s
gulp dist --fortesting: ~3m 30s

Fixes ampproject#9640

* add amp-analytics Facebook Pixel support (ampproject#9449)

* add amp-analytics Facebook Pixel support

* add amp-analytics Facebook Pixel support

* fix test on extensions/amp-analytics/0.1/test/vendor-requests.json

* add Facebook Pixel standard events https://developers.facebook.com/docs/ads-for-websites/pixel-events/v2.9#events

* Update vendors.js

fix conflicts

* Update vendors.js

fix conflicts

* Update vendor-requests.json

fix conflicts

* Update vendor-requests.json

fix conflicts

* Update vendor-requests.json

* Update analytics.amp.html

* Update analytics-vendors.amp.html

* Update vendors.js

* Update analytics-vendors.amp.html

* fix lint error

* update

* facebook pixel

* Update vendor-requests.json

* Update analytics.amp.html

* fix lint error

* fix lint error

*  fix alphabetical order

* Update yarn.lock

* AMP-ad supports Seznam Imedia ad network (ampproject#8893)

* AMP-ad supports Seznam Imedia ad network

* Fixed requested changes

* Fixed requested changes

* Render API implemented

* Improved placing ads

* Fixed requested changes

* Render API fixed context
* Renamed IM.cz to Imedia
* Described JSON parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants