Conversation
Added a attribute to the attachLinear method in AttachPodAds. Right now we set it as 5 seconds but it could be changed to another value or a percentage. We could also make it so that we look for a sent in skipoffset value from the sent in params.
Nfrederiksen
left a comment
Nfrederiksen
left a comment
There was a problem hiding this comment.
Am I missing something here? (:
I only see one line changed. Are there any other commits to be check in?
The PR description makes it sound like you've provided a way to set the skipoffset or?
I added so you can send in a value for the parameter skipoffset plus a format checker for the parameter. Also added a skipoffset value in the example response in Swagger.
|
Hello my apologies, I did not understand that the skipoffset was supposed to be a parameter that the user could input a value to. I've changed it now! I added a isValidSkipOffset in the file vast-maker.js but I'm not sure that it's the best place to put it in! Just holla at me if I should move it elsewhere! |
Nfrederiksen
left a comment
Nfrederiksen
left a comment
There was a problem hiding this comment.
Firstly, the ticket could be a little more clearer and have a little more context in it before handing it off to you guys ^^' I see that now. Also, the placement of the isValidSkipOffset() function is ok 👍However, I'd rename the function to 'getSkipOffsetValue' or similar as an 'isValidXXX' function is typically expected to only output true or false.
Secondly, you will need to make sure the vmap-maker can also access the skip offset feature, as we want the VAST's inside the VMAP to include the skipoffset attributes too. Will require a schema update too.
Lastly, I'd like to request some changes to one of the input formats.
I like the idea of having one parameter for skipoffset but with two possible input formats. But I believe it would be better if a user could enter a number of seconds rather than a timecode string, i.e. "5" instead of "00:00:05". Since, it is most likely that a user will only want a few seconds as an offset and it would be cumbersome to have to enter the extra "00:00:" every time.
eg. you should be able to set the parameter either like &skip=50% or &skip=5 where 5 will need to be translated into a timecode string. You will need to write a function for that. and possibly update the regex part in the validation step. (^_^)
Other than that looks good
|
Thank you for the feedback @Nfrederiksen. I'll try and have these changes implemented by tomorrow! You've been a huge help for the student development team this iteration, thank you so much! 🙏🏼 |
Added so that vmap-maker sends in skipoffset as a param into the VastBuilder. I also added a skipoffset variable into the defaultConfigs in vmap-maker.js.
|
Heya, I added the skipoffset to the vmap-maker by storing the skipoffset in the params that are sent in to the VastBuilder. I also updated it so that a parameter for skipoffset exists in the vmap endpoint. I also changed the name of isValidSkipOffsetValue to getSkipOffsetValue and added so that you can write the skipoffsets in seconds without having to write it in the "hh:mm:ss" format! |
utils/vmap-maker.js
Outdated
|
|
||
| const breakpoints = params.breakpoints ? params.breakpoints.split(",").filter((item) => !isNaN(Number(item))) : []; | ||
| if (params.preroll) { | ||
| //console.log("PARAMS: ",params.generalVastConfigs); |
There was a problem hiding this comment.
This may be removed/cleaned up
utils/vast-maker.js
Outdated
| * | ||
| */ | ||
| function VastBuilder(params) { | ||
| console.log("VastBuilder params: ", params); |
There was a problem hiding this comment.
Remove/Clean up this
Nfrederiksen
left a comment
Nfrederiksen
left a comment
There was a problem hiding this comment.
In Summary, Other than some need for clean up and timecode format disabling. The PR looks good.
utils/vast-maker.js
Outdated
| return lowest; | ||
| } | ||
|
|
||
| // Validate that params.skipoffset is a valid VAST skipoffset value ("x%" or "hh:mm:ss"). |
There was a problem hiding this comment.
You can remove this comment too
utils/vast-maker.js
Outdated
| // "seconds" | ||
| const integerSecondsRegex = /^\d+$/; | ||
|
|
||
| if (timeFormatRegex.test(skipoffset) || percentageFormatRegex.test(skipoffset)){ |
There was a problem hiding this comment.
Let's keep it to just two valid formats and remove the timecode format in this function, eg. "00:00:03" should not set any skipoffset
|
Did some cleaning up in the code and also removed the "hh:mm:ss" format check in the getSkipOffsetValue! ^^ |
Nfrederiksen
left a comment
Nfrederiksen
left a comment
There was a problem hiding this comment.
Look Good To Me!
Sorry that the final review took some time.
3 participants
We added an attribute to the attachLinear method in AttachPodAds. We set it as 5 seconds but it could be changed to another value or a percentage. Right now it's very straightforward, every ad will be skippable after 5 seconds, assuming they're longer than 5 seconds. We could also make it so that we look for a skipoffset value from the ads properties and if they have one we'll add it to the params of the builder so you can choose skipoffset value or not have one at all.
Also a small sidenote, the skipoffset attribute for the tag is not part of VAST 2.0. The builder will still create a VAST 2.0 XML to the official documentation, that is to say it wont add the attribute to the tag if you choose VAST 2.0.