-
Notifications
You must be signed in to change notification settings - Fork 63
Add replay action and played state to audio track #301
Conversation
if (prevStatus === 'played' && status === 'playing') { | ||
// If going from played to playing, then reset the time to the beginning. Let the regular logic handle actually triggering the playing of the audio | ||
audioEl.value.currentTime = 0 | ||
} |
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.
Great. This makes a lot of sense to me, that internally this only cares about the current playtime rather than explicitly triggering events. "If we're transitioning from 'playing' to 'played', set the timestamp to 0" is quite logical.
const handleSeeked = () => { | ||
if (status.value === 'played') { | ||
status.value = 'paused' | ||
} | ||
} |
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 like this!
@@ -146,6 +151,10 @@ export default { | |||
|
|||
currentTime.value = audioEl.value.currentTime | |||
duration.value = audioEl.value.duration | |||
|
|||
if (currentTime.value >= duration.value) { | |||
emit('finished') |
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.
👍
@sarayourfriend You can go ahead and write the tests, this is a great approach to this work and you made improvements across the board 👍 |
@zackkrida I had started trying to write tests for this on Friday but... I ran into a big issue, which is that I'm not sure how to set up the audio element inside the tests with an actual duration and currentTime value that the component can run against without making a network request 😱 I was going to look into whether we could include a test file into the repository that |
Unfortunately it's not possible to actually write unit tests against this... because we're operating against a real HTMLAudioElement, which isn't fully implemented by JSDom. For example, it doesn't actually |
@sarayourfriend in that case...LGTM! |
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 works great!
Fixes
Fixes #289 by @zackkrida
Description
Adds a new
played
state to the AudioTrack triggered when the current time of the audio matches or exceeds the duration of the audio. When seeking during this state, it toggles back topaused
. When hitting the play/pause button during the state, it will restart the audio from the beginning.Gravacao.de.Tela.2021-10-08.as.08.43.26.mov
I tried to implement this in the least intrusive way possible... In the course of that I added two new events to the AudioControlled (
finished
andseeked
). Theseeked
event is probably the most problematic as it doesn't emit the timestamp or anything like that, which you could argue would make sense for aseeked
event to do. But it wasn't necessary for this particular application of that event I was sort of thinking of a YAGNI kind of thing, to keep the event as minimal as possible for the time being. If in the future we need to add the timestamp it would be trivial to do (but I'm also open to adding it now if folks think it would make more sense that way, even though it's not needed for this particular instance).I tried to think of a few different approaches to solving this problem but then all eventually hit dead ends... this was the least intrusive version of these changes I could come up with.
Testing Instructions
Checkout this PR and run
npm run storybook
and navigate to/?path=/story/components-audio-track--default-story
. Play around with the audio track and the new replay functionality and verify that the existing behavior continues to work (especially seeking) and that the new replay behavior works and makes sense.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin