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.

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

add creative support w/out duration for wrapper tags #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pbwilmot
Copy link

@pbwilmot pbwilmot commented Jul 5, 2016

Goal of this PR : include the ability to add creatives with tracking events but not set a duration or mediafile for wrapper tags

excerpt from spec :
2.4.1.4 Linear Creative Format within a Wrapper
The most important difference between a Wrapper Linear creative and an Inline one is that a Wrapper
Linear creative is absent of any media files. The only elements allowed in a Wrapper Linear creative are
VideoClicks and TrackingEvents. These tracking elements enable tracking data to be
collected at the Wrapper for any events that occur in the Inline Linear creative that is served following
the Wrapper.

@pbwilmot
Copy link
Author

pbwilmot commented Jul 6, 2016

you also might want to update to current versions of 'tap' and 'libxmljs' since the versions that are used in this project are pretty outdated and wont build anymore.

@tbuchok
Copy link
Owner

tbuchok commented Jul 6, 2016

@pbwilmot thanks for the clear PR and helpful feedback! i'm a bit busy this morning but will take a look and merge / npm publish within the next day or so. are you blocked meantime?

you mentioned updating the deps, good call. i suppose it might be time to bump the major version on this and set 2.x.x as vengine: 4.^ only, yea?

thanks again and i'll be in touch as we make progress on this.

@tbuchok
Copy link
Owner

tbuchok commented Jul 8, 2016

@pbwilmot one thing i'm noticing in this is that the test file was updated, but no new tests were added. are you able to add some tests to handle this specific scenario?

if not, i can add merge this and then add them before publishing to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants