Skip to content
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

Refine playback #29

Merged
merged 16 commits into from
Apr 2, 2017
Merged

Refine playback #29

merged 16 commits into from
Apr 2, 2017

Conversation

stevenpetryk
Copy link

@stevenpetryk stevenpetryk commented Apr 2, 2017

This adds some enhancements to playback

  • Added ability to play and pause streams
  • Added ability to skip to random points in streams
  • Gracefully handle when a stream ends
  • Add proof-of-concept audio playback (with Khan Academy audio)
  • Optimized skipping forward (just draws line-by-line, not point-by-point)
  • Added a loading state to the stream view

Greg, I'm putting you in this PR, because you told me you haven't built the frontend in 2 months. ;)

setTimeout(() => {
dispatch(fetchStream(streamId))
dispatch(fetchStreamData(streamId)).then(resolve)
}, 1000)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just makes the frontend retry until the upload to S3 is complete. In testing, it rarely retries more than once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should be fine! As long as it really isn't a noticeable thing. What about if the upload fails for whatever reason? Should we worry about that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should worry about that, but I don't think the frontend should.

this.position = clamp(initialPosition + now - start, 0, this.duration)
this.emit('POSITION_CHANGE', this.position)

if (this._draw()) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_draw() returns false when there's nothing left to draw (i.e., once the stream is done playing).

}

return this.parsedLines[index]
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lazy parsing ftw!


setPosition (position) {
const wasPlaying = this.playing
if (this.playing) this.pause()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be wasPlaying?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (wasPlaying) this.pause()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works too! Probably more descriptive. Thanks.


EventEmitter(DrawManager.prototype)

/* global d3, requestAnimationFrame, cancelAnimationFrame */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments tell the linter "ay, these are defined in the global scope, so don't get mad at me for referencing undefined variables"

Copy link
Contributor

@maxcell maxcell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally love it! It would be neat to have playback reverse/forward by 5 sec using the arrows but super low priority and use case.

@stevenpetryk stevenpetryk merged commit 4be1cca into develop Apr 2, 2017
@stevenpetryk stevenpetryk deleted the feat/playback-refinements branch April 2, 2017 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants