Conversation
|
Assigning all three of you for review because I'm not sure who actually has time for a review now; if you're swamped, please feel free to ignore this and let the others review. |
|
FYI @poorvasingal |
| element.style.display = 'none'; | ||
| document.body.appendChild(element); | ||
| element.click(); | ||
| document.body.removeChild(element); |
There was a problem hiding this comment.
This bit of code (creating a hidden element so that we can click on it to start a download I think?) feels like it could use a comment explaining itself.
There was a problem hiding this comment.
Sure thing! Is there a more standard way to initiate a user download from JS?
There was a problem hiding this comment.
Not that I know of, but I think this is enough of a "trick" that many won't be familiar with it/understand why this is being done when they see it in code.
| return false; | ||
| }; | ||
|
|
||
| checkVideo = () => { |
There was a problem hiding this comment.
I think for methods like this where they're not being passed to an event handler, they can be true member functions, i.e. checkVideo() { instead of arrow functions
| return; | ||
| } | ||
|
|
||
| if (this.state.checkVideoUntilSuccessTimeout) { |
There was a problem hiding this comment.
I would guess that checkVideoUntilSuccessTimeout could be a member variable instead of a state variable, i.e. this.checkVideoUntilSuccessTimeout
…sn't actually impact the state of the component
…an event handler, it doesn't need to be bound
Depends on code-dot-org/p5-replay#39
User experience when the video is either already generated, or quick to generate:
User experience when the video is taking absurdly long to generate:
Follow-up work in a future PR: