Update duration to check for second value in files array #3500
Conversation
| if (this.firstNode.kind === 'audio' || this.firstNode.kind === 'video') { | ||
| return this.nodeFiles.filter( | ||
| audioVideoFiles = this.nodeFiles.filter( | ||
| file => file.file_format === 'mp4' || file.file_format === 'mp3' |
There was a problem hiding this comment.
I know you didn't change this, but it looks worthwhile to fix-- it's excluding the newly added .webm format. To go one step further, for posterity, I would suggest using the presets information on what formats are video or audio resources
There was a problem hiding this comment.
I actually wonder if it's necessary to include this check here, now that I revisit it, since I believe the presets are already checked earlier in upload?
There was a problem hiding this comment.
@bjester I've removed for now, based on my understanding of what's happening in upload, but if you think keeping this check (and updating to include .webm, using presets) here is helpful, I will add in a modified version
There was a problem hiding this comment.
Yeah I thought the same about keeping the check, and what I guessed was that since this uses nodeFiles, that it could run into subtitle files too?
There was a problem hiding this comment.
great point! I've updated @bjester
contentcuration/contentcuration/frontend/channelEdit/components/edit/DetailsTabView.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/channelEdit/components/edit/DetailsTabView.vue
Outdated
Show resolved
Hide resolved
|
Thanks @bjester - your comments are really helpful. I'll plan to implement these updates and re-request your review tomorrow morning :) |
… files that support captioning, subtitles, etc.
| FormatPresetsMap.get(FormatPresetsNames.HIGH_RES_VIDEO).allowed_formats | ||
| ); | ||
| allowedFileTypes.push(FormatPresetsMap.get(FormatPresetsNames.AUDIO).allowed_formats); | ||
| allowedFileTypes = allowedFileTypes.flat(); |
There was a problem hiding this comment.
TIL .flat() exists on Array.prototype
There was a problem hiding this comment.
A slight optimization would be to only create this allowedFileTypes array once, outside of this function scope, but I think it's unlikely to have major performance differences
Summary
Description of the change(s) you made
This updates the duration value to check for both values in the potential array (high and low resolution video), and uses the prop rather than a data calculation.
Fixes #3472
Manual verification steps performed
Screenshots (if applicable)
studio-video-duration-updates.mp4
Does this introduce any tech-debt items?
Not that I am aware of
Reviewer guidance
How can a reviewer test these changes?
You can follow the manual QA steps listed above
Are there any risky areas that deserve extra testing?
There was one scenario that seemed a bit wonky, but I haven't be able to reproduce consistently.
Comments
It's possible there might be edge cases I'm not thinking of, of why this solution wouldn't be a good idea?
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)