[Component] GPT component #26

JGAntunes · 2017-06-23T20:38:30Z

An ad component and its related parts, already supporting header biding also. It's still a work in progress, probably lacking further tests for the ad-manager.

sericaia · 2017-06-26T16:24:45Z

@craigbilner can you please review and merge?

craigbilner

In general I think:

Testing needs to be a little clearer with a lot less theatre around setting everything up. It's very unit test heavy and would be nice to have integration tests to verify the whole piece

The components don't seem like they all belong under the one gpt package? There's clearly a separation between the managers and the react component. The story has a Wrapper component which seems to glue everything together, how does this work in the real world? Would you not import the components separately?

I appreciate working within the storybook with the iframe is a bit of a hassle but would be good to see multiple ads and making sure the managers are doing their jobs? Also do we need different types/sizes of ads?

craigbilner · 2017-06-27T16:11:44Z

packages/gpt/ad-manager.js

+import gptManager from "./gpt-manager";
+import pbjsManager from "./pbjs-manager";
+
+function AdManager(options = {}) {


Can we use ES2015 classes across the board for consistency/readability etc.?

craigbilner · 2017-06-27T16:12:25Z

packages/gpt/ad-manager.js

+import pbjsManager from "./pbjs-manager";
+
+function AdManager(options = {}) {
+  if (!(this instanceof AdManager)) {


https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target

Should we use this anyway?

I think we're quite far from serving ES2015+ code to the browser? So there'll be a transpile step - https://www.npmjs.com/package/babel-plugin-transform-class

craigbilner · 2017-06-27T16:12:54Z

packages/gpt/ad-manager.js

+  }
+
+  this.adQueue = [];
+  this.adUnit = options.adUnit;


could use Object.assign(this, {...props}) here

craigbilner · 2017-06-27T16:20:54Z

packages/gpt/ad-manager.js

+  this.initialised = false;
+}
+
+AdManager.prototype.isReady = function isReady() {


Can change to a get

craigbilner · 2017-06-27T16:23:30Z

packages/gpt/ad-manager.js

+  gptManager.loadScript();
+  pbjsManager.loadScript();
+
+  series(


Do we need these libraries here? What's their value add?

Well.. async helps to avoid callback hell and to better structure and organise your code and thinking. So, I would say yes, we need.

craigbilner · 2017-06-27T16:42:01Z

packages/gpt/ad-manager.js

+AdManager.prototype._pushAdToGPT = function pushAdToGPT(
+  adSlotId,
+  sizingMap,
+  options = {}


craigbilner · 2017-06-27T16:46:35Z

packages/gpt/ad-manager.test.js

+  let AdManager;
+
+  beforeEach(() => {
+    const window = new JSDOM().window;


I'd prefer if we didn't do any integration testing like this at all. Everything ideally should be injectable and mockable through HOCs or other patterns. Also I'd probably make more use of jest/enzyme here to render into real DOMs and mock things in perhaps a more elegant way

craigbilner · 2017-06-27T16:47:11Z

packages/gpt/ad-manager.test.js

+  it("constructor returns an AdManager instance with correct props", () => {
+    const adManager = AdManager(managerOptions);
+    expect(adManager).toBeInstanceOf(AdManager);
+    expect(adManager.adUnit).toBe(managerOptions.adUnit);


Ideally we wouldn't have so many asserts for a single test and at the very least have descriptions so a failure can easily be identified

craigbilner · 2017-06-27T17:00:37Z

packages/gpt/gpt.test.js

+    .create(<GPT adManager={adManager} code="ad-header" />)
+    .toJSON();
+
+  expect(tree).toBeTruthy();


can we snapshot test?

craigbilner · 2017-06-27T17:00:56Z

packages/gpt/gpt.web.js

+
+  render() {
+    const props = this.props;
+    return <div id={props.code} />;


this.props.code?

coveralls · 2017-07-04T10:48:08Z

Changes Unknown when pulling da81ecc on feature/gpt-component into ** on master**.

coveralls · 2017-07-04T11:05:22Z

Changes Unknown when pulling 71af2ae on feature/gpt-component into ** on master**.

JGAntunes · 2017-07-04T13:04:16Z

I think all the needed changes have been addressed, can you confirm @craigbilner?

coveralls · 2017-07-04T13:06:39Z

Changes Unknown when pulling 431d9a1 on feature/gpt-component into ** on master**.

coveralls · 2017-07-04T13:25:14Z

Changes Unknown when pulling 44118f3 on feature/gpt-component into ** on master**.

craigbilner · 2017-07-05T13:42:58Z

packages/gpt/ad-composer.js

+    super(props);
+
+    this.adManager = new AdManager({
+      networkId: "25436805",


should be a prop for different environments

craigbilner · 2017-07-05T14:12:07Z

packages/gpt/ad-manager.js

+  }
+
+  display(callback) {
+    if (!this.initialised) {


I think we can make an assumption that this won't happen? Can we remove the defensive code to keep things cleaner? Can add a README if devs need to understand how to use it better

craigbilner · 2017-07-05T14:12:39Z

packages/gpt/ad-manager.js

+      slot.addService(this.gptManager.googletag.pubads());
+      slot.defineSizeMapping(this._generateSizings(sizingMap));
+
+      // const targeting = omit(options, 'iuPartsSuffix');


craigbilner · 2017-07-05T14:15:31Z

packages/gpt/gpt-manager.js

+
+    const googletag = this.googletag;
+
+    googletag.cmd.push(() => {


this all seem quite mysterious, are there some docs we can point to with a comment?

Well, all the config setup calls are commented (except probably for the first one, whose comment doesn't make much sense (fixing it now). I can also leave a comment with a link for https://developers.google.com/doubleclick-gpt/reference#googletagpubadsservice

craigbilner · 2017-07-05T14:23:52Z

packages/gpt/pbjs-manager.js

+    const pbs = d.createElement("script");
+    pbs.async = true;
+    pbs.type = "text/javascript";
+    pbs.src = "https://www.thetimes.co.uk/d/js/vendor/prebid.min-4812861170.js";


can we raise an issue to revisit this, as it's open source shouldn't we just bundle the code as a normal vendor dependency?

coveralls · 2017-07-05T17:44:02Z

Coverage decreased (-23.1%) to 76.866% when pulling eb06190 on feature/gpt-component into 3d5f45b on master.

coveralls · 2017-07-06T02:37:44Z

Coverage decreased (-15.8%) to 84.211% when pulling 78b87a7 on feature/gpt-component into 3d5f45b on master.

coveralls · 2017-07-06T10:38:13Z

Coverage decreased (-15.7%) to 84.286% when pulling febfa33 on feature/gpt-component into 3d5f45b on master.

coveralls · 2017-07-06T12:40:08Z

Coverage decreased (-13.6%) to 86.364% when pulling 937e4f3 on feature/gpt-component into 3d5f45b on master.

coveralls · 2017-07-06T15:57:17Z

Coverage decreased (-9.09%) to 90.909% when pulling 1e92982 on feature/gpt-component into 3d5f45b on master.

coveralls · 2017-07-06T16:11:09Z

Coverage decreased (-9.09%) to 90.909% when pulling 1e1211a on feature/gpt-component into 3d5f45b on master.

coveralls · 2017-07-06T16:22:22Z

Coverage decreased (-9.09%) to 90.909% when pulling 5a4e3af on feature/gpt-component into 3d5f45b on master.

coveralls · 2017-07-06T17:25:33Z

Coverage decreased (-2.9%) to 97.129% when pulling c6edee6 on feature/gpt-component into 3d5f45b on master.

Storybook relies on iframes to render stories, gpt map sizes, used to build responsive ads don't seem to work with it. This hack solves it for now until further developments on storybookjs/storybook#862

…yway

coveralls · 2017-07-07T14:47:58Z

Coverage decreased (-1.8%) to 98.182% when pulling c1b1f82 on feature/gpt-component into 966d43d on master.

coveralls · 2017-07-07T15:56:53Z

Coverage decreased (-1.8%) to 98.182% when pulling 2a3c4e1 on feature/gpt-component into 966d43d on master.

coveralls · 2017-07-07T16:09:23Z

Coverage decreased (-1.8%) to 98.182% when pulling bf64aa5 on feature/gpt-component into 966d43d on master.

coveralls · 2017-07-07T16:46:46Z

Coverage decreased (-2.4%) to 97.605% when pulling fe0dfb8 on feature/gpt-component into 966d43d on master.

coveralls · 2017-07-07T16:51:57Z

Coverage decreased (-1.8%) to 98.204% when pulling acb59a9 on feature/gpt-component into 7852705 on master.

coveralls · 2017-07-07T18:47:03Z

Coverage decreased (-2.4%) to 97.605% when pulling fe0dfb8 on feature/gpt-component into 966d43d on master.

* feature: add gpt component with support for header biding * fix: deal with some issues in the gpt story * fix: render ad inside storybook Storybook relies on iframes to render stories, gpt map sizes, used to build responsive ads don't seem to work with it. This hack solves it for now until further developments on storybookjs/storybook#862 * chore: document gpt story hack * chore: add tests for gpt, pbjs and ad managers * chore: add more tests for ad manager * add more tests to ad-manager * fixes here and there (console.log, typos, etc) * classes instead of functions * corrected most comments [WIP] * snapshot test * change story to render two ads in article page * quick refactor on ad manager test * alternative using react broadcast * fix tests [WIP] * snapshot tests; pbjs config; tests adapted * fix: add missing react-broadcast dep * chore: redo initialisation checks on ad, gpt and pbjs managers * remove old test * fix: jest config on gpt component * chore: remove unneeded JSDOM dev dep * fix: add section as a prop of ad composer * fix: return on all callbacks * chore: lint gpt component * fix: network id should be a prop of AdManager * chore: add comments on gpt config * chore: use promises on the gpt, pbjs and ad managers * chore: remove errors on improper class usage * CC linting err fixed; adUnit as prop * remove new.target as class constructors need to be called with new anyway * chore: increase coverage * one more test * chore: update jest configuration * prebid settings unit tests * fix: change test assumption titles * fix: test message to remember to turn ad blocker off * fix: throw error if slot does not exist * fix: use storybook url and remove transform

JGAntunes assigned sericaia Jun 23, 2017

sericaia approved these changes Jun 26, 2017

View reviewed changes

foliveira requested a review from craigbilner June 27, 2017 11:18

craigbilner suggested changes Jun 27, 2017

View reviewed changes

sericaia force-pushed the feature/gpt-component branch 4 times, most recently from 076bafc to d8a014f Compare June 30, 2017 14:31

JGAntunes changed the title ~~[WIP][Component] GPT component~~ [Component] GPT component Jul 4, 2017

craigbilner reviewed Jul 5, 2017

View reviewed changes

JGAntunes force-pushed the feature/gpt-component branch from 44118f3 to eb06190 Compare July 5, 2017 17:41

JGAntunes added 5 commits July 7, 2017 15:42

feature: add gpt component with support for header biding

3182e19

fix: deal with some issues in the gpt story

eb9ebe2

fix: render ad inside storybook

537546b

Storybook relies on iframes to render stories, gpt map sizes, used to build responsive ads don't seem to work with it. This hack solves it for now until further developments on storybookjs/storybook#862

chore: document gpt story hack

412e034

chore: add tests for gpt, pbjs and ad managers

ef55e14

JGAntunes and others added 13 commits July 7, 2017 15:44

fix: add section as a prop of ad composer

3605006

fix: return on all callbacks

952ca9f

chore: lint gpt component

12b0b14

fix: network id should be a prop of AdManager

1760ba1

chore: add comments on gpt config

a6c617a

chore: use promises on the gpt, pbjs and ad managers

fafa73f

chore: remove errors on improper class usage

00f7878

CC linting err fixed; adUnit as prop

0e75b03

remove new.target as class constructors need to be called with new an…

e1f930c

…yway

chore: increase coverage

39999e6

one more test

bcc96e4

chore: update jest configuration

b5856ed

prebid settings unit tests

c1b1f82

sericaia force-pushed the feature/gpt-component branch from c6edee6 to c1b1f82 Compare July 7, 2017 14:45

fix: change test assumption titles

2a3c4e1

fix: test message to remember to turn ad blocker off

bf64aa5

fix: throw error if slot does not exist

acb59a9

sericaia force-pushed the feature/gpt-component branch from fe0dfb8 to acb59a9 Compare July 7, 2017 16:49

fix: use storybook url and remove transform

ba307b2

craigbilner approved these changes Jul 10, 2017

View reviewed changes

craigbilner merged commit 59b6201 into master Jul 10, 2017

craigbilner deleted the feature/gpt-component branch July 10, 2017 09:50

[Component] GPT component #26

[Component] GPT component #26

Conversation

JGAntunes commented Jun 23, 2017

sericaia commented Jun 26, 2017

craigbilner left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

JGAntunes commented Jul 4, 2017

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 5, 2017

coveralls commented Jul 6, 2017

coveralls commented Jul 6, 2017

coveralls commented Jul 6, 2017

coveralls commented Jul 6, 2017

coveralls commented Jul 6, 2017

coveralls commented Jul 6, 2017

coveralls commented Jul 6, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017 • edited Loading

craigbilner left a comment •

edited

Loading

coveralls commented Jul 7, 2017 •

edited

Loading