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

Creative size #311

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

Creative size #311

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3721a1c
adding creativeSize attribute to pass in a VAST size
Jun 6, 2017
b9649ab
meant to leave it at 640x480
Jun 6, 2017
2037cdb
removing unnecessary PropTypes and making the size attribute optional
Jun 6, 2017
44d65e9
fixing the proptype things
Jun 6, 2017
f22a4ea
moving the let statement to the right function
Jun 6, 2017
b4ee622
seems like a duplicate test
Jun 12, 2017
9af05ae
Fixing tests
spra85 Jun 13, 2017
6315084
Test for: It allows creativeSize for preroll vast tag URL to be overr…
spra85 Jun 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion elements/bulbs-video/components/revealed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ export default class Revealed extends React.Component {
let specialCoverage = this.props.targetSpecialCoverage || 'None';
let autoplayInViewBool = typeof this.props.autoplayInView === 'string';

// Allowing creativeSize to be passed in in root.js
let creativeSize = this.props.creativeSize;

@spra85 spra85 Jun 6, 2017

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 would make this an optional attribute, with a default value of 640x480, so simply change this to:

let creativeSize = this.props.creativeSize || '640x480';

@kjacovino kjacovino Jun 6, 2017

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.

can do!


let videoAdConfig = 'None';
if (this.props.disableAds || this.props.video.disable_ads) {
videoAdConfig = 'disable-ads';
Expand All @@ -79,6 +82,8 @@ export default class Revealed extends React.Component {
dimensions
);



// Making assignment copies here so we can mutate object structure.
let videoMeta = Object.assign({}, this.props.video);
videoMeta.hostChannel = hostChannel;
Expand Down Expand Up @@ -171,7 +176,7 @@ export default class Revealed extends React.Component {
let type;

// See docs (https://support.google.com/dfp_premium/answer/1068325?hl=en) for param info
baseUrl += '?sz=640x480';
baseUrl += `?sz=${this.props.creativeSize}`;

@spra85 spra85 Jun 6, 2017

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.

So, you should either find a way to use the creativeSize you defined above in the let assignment or do what you're doing here but otherwise the assignment above isn't used.

@kjacovino kjacovino Jun 6, 2017

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.

so if I removed the PropTypes line, does that mean this gets to stay?

baseUrl += `&iu=/4246/${window.Bulbs.settings.DFP_SITE_CODE}`;
baseUrl += '&impl=s';
baseUrl += '&gdfp_req=1';
Expand Down Expand Up @@ -361,6 +366,7 @@ Revealed.propTypes = {
autoplay: PropTypes.bool,
autoplayInView: PropTypes.string,
autoplayNext: PropTypes.bool,
creativeSize: PropTypes.object.string,
controller: PropTypes.object.isRequired,
defaultCaptions: PropTypes.bool,
disableAds: PropTypes.bool,
Expand Down
2 changes: 2 additions & 0 deletions elements/bulbs-video/elements/rail-player/components/root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export default class Root extends React.Component {
<Revealed
disableSharing={true}
muted={true}
creativeSize='640x480'
targetHostChannel='right_rail'
defaultCaptions={true}
{...this.props}
Expand All @@ -55,6 +56,7 @@ Root.displayName = 'RailPlayerRoot';

Root.propTypes = {
channel: PropTypes.string,
creativeSize: PropTypes.string,
recircUrl: PropTypes.string.isRequired,
targetCampaignId: PropTypes.string,
targetSpecialCoverage: PropTypes.string,
Expand Down
1 change: 1 addition & 0 deletions elements/bulbs-video/elements/rail-player/rail-player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ Object.assign(RailPlayer, {
},
propTypes: {
channel: PropTypes.string.isRequired,
creativeSize: PropTypes.string,

@spra85 spra85 Jun 6, 2017 •
edited
Loading

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 don't think you actually need the creativeSize propType defined here.

@kjacovino kjacovino Jun 6, 2017

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.

true statement, still works without it

recircUrl: PropTypes.string.isRequired,
src: PropTypes.string.isRequired,
targetCampaignId: PropTypes.string,
Expand Down