-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(web-component): added auto pause feature #240
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment for now as I try to understand the code. I'll test it and see if it works well for me, and maybe Marc can test it too.
const sentence = | ||
paragraphs[paragraphs.length - 1].querySelector("s:last-of-type"); | ||
const word = sentence.querySelector("w:last-of-type"); | ||
this.endOfPageTags[word.id] = parseFloat(word.getAttribute("dur")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really weird, why is the duration of the last word becoming the ID for the end of page tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am creating a list of the last word spoken on each page, using the ID as the key and the duration of the word to help calculate when to pause the audio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now, you're using this duration to determine when to pause exactly.
if (!this.isAutoPaused) | ||
setTimeout( | ||
() => this.pause(), | ||
this.endOfPageTags[el_tag] * 800 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this 800 constant value mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested, and it seems to work well, except this constant is funky: we're setting the timeout at 800ms per second of duration or the last word in the page. If we need to stop before the end of the last word, I'd rather see us doing something like this.endOfPageTags[el_tag] * 1000 - 50
and document why we want to stop 50ms (or whatever number we choose) before the end of this last word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the best "guess estimate" to help convert the duration of the word into a timeout value. Ideally, I would convert the duration directly to milliseconds; however, this bit of code activates during the silence before the beginning of the word in the audio. Maybe Aidan knows the exact point in the silence at which the event is emitted. Then, we can use that in a formula for the timeout value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change seems good, modulo my comments above about the exact delay, but this breaks unit testing. npx nx test:once web-component
fails in two of the six specs we run. You can see the logs at https://github.com/ReadAlongs/Web-Component/actions/runs/8281122034/job/22672362698 which I reproduced locally as well.
fixed. |
I got it to bug with a four page readalong, I've emailed you the files I used. |
A feature to pause the audio at the end of each page was added. For now, it is activated via an attribute