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

Feat: add 'replay' option to the PlayToggle component. #5531

Merged
merged 10 commits into from Nov 2, 2018

Conversation

@gjanblaszczyk
Copy link
Contributor

@gjanblaszczyk gjanblaszczyk commented Oct 26, 2018

Description

A proposed fix for #4802

Specific Changes proposed

I added 'replay' option to the PlayToggle component.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors
Copy link
Member

@misteroneill misteroneill left a comment

Looks good to me as-is. Especially nice that you updated the documentation!

I wonder if this is a situation where we'd want to have a replay() getter/setter method? That could make this toggle-able at runtime. Thoughts @gkatsev?

@gjanblaszczyk
Copy link
Contributor Author

@gjanblaszczyk gjanblaszczyk commented Oct 31, 2018

Thanks @misteroneill,
I didn't consider using setter and getter for the replay functionality but I can add it without any problems if you want it.

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Oct 31, 2018

Yeah, I don't think it's worth it. We can add it in later if someone requests it.

gkatsev
gkatsev approved these changes Nov 2, 2018
@gkatsev gkatsev merged commit f178458 into videojs:master Nov 2, 2018
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants