Chore: Support auto captions in legacy and edited captions #825
Conversation
|
Verified that @jeremypress has signed the CLA. Thanks for the pull request! |
src/lib/viewers/media/DashViewer.js
Outdated
| if (!skillsCards) { | ||
| return; | ||
| parseTextCues(transcriptCards) { | ||
| if (!transcriptCards) { |
There was a problem hiding this comment.
Could we use default parameter values for these functions in order to cut down on some of the guard clauses? Something like parseTextCues(transcriptCards = [])?
src/lib/viewers/media/DashViewer.js
Outdated
| * | ||
| * @return {void} | ||
| * @param {Object} transcriptCards - Transcript cards |
There was a problem hiding this comment.
Should this be @param {Array}? It looks like .find is used on it below.
src/lib/viewers/media/DashViewer.js
Outdated
| * @param {Object} skillsData - updated skills data | ||
| * @return {void} | ||
| */ | ||
| editAutoGeneratedCaptions(skillsData) { |
There was a problem hiding this comment.
It looks like there's some duplication between this function and loadAutoGeneratedCaptions. Should there be a third method that both of these call? I also think update might be more clear than edit, since no explicit user interaction is involved.
There was a problem hiding this comment.
Yep, will clean up in the next commit. Agreed that's a better name.
src/lib/viewers/media/DashViewer.js
Outdated
| this.autoCaptionDisplayer.append(textCues); | ||
| this.player.configure({ textDisplayFactory: this.autoCaptionDisplayer }); | ||
| if (areAutoCaptionsVisible) { | ||
| this.autoCaptionDisplayer.setTextVisibility(true); |
There was a problem hiding this comment.
Could we call this.autoCaptionDisplayer.setTextVisibility(areAutoCaptionsVisible) to clarify that we're preserving the user's preference? What do you think?
There was a problem hiding this comment.
I don't think there's any harm to calling it again with false if they are already off, and it'll make the code cleaner.
src/lib/viewers/media/DashViewer.js
Outdated
| } | ||
|
|
||
| const transcriptCard = skillsCards.find((card) => card.skill_card_type === 'transcript'); | ||
| const textCues = []; |
There was a problem hiding this comment.
Since textCues isn't being used until after the if, why not declare it there?
src/lib/viewers/media/DashViewer.js
Outdated
| this.skillsData = skillsData; | ||
|
|
||
| const textCues = this.parseTextCues(skillsCards); | ||
|
|
||
| if (textCues.length > 0) { | ||
| this.autoCaptionDisplayer = new shaka.text.SimpleTextDisplayer(this.mediaEl); |
There was a problem hiding this comment.
Could you reuse this, if it's already been created beforehand?
IE) this.autoCaptionDisplayer = this.autoCaptionDisplayer || new SimpleTextDisplayer(...)
src/lib/viewers/media/DashViewer.js
Outdated
|
|
||
| const textCues = this.parseTextCues(skillsData); | ||
|
|
||
| if (textCues.length > 0) { |
There was a problem hiding this comment.
Guard to make sure that autoCaptionDisplayer exists?
src/lib/viewers/media/DashViewer.js
Outdated
| createTextCues(transcriptCard) { | ||
| const entries = getProp(transcriptCard, 'entries', []); | ||
| const textCues = []; | ||
|
|
||
| entries.forEach((entry) => { |
There was a problem hiding this comment.
Minor: If desired, you could convert this to a .map call to avoid the temporary textCues variable and mutation that goes with it.
Will add tests/cleanup tomorrow