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

Issue/254 loop component #638

Merged
merged 8 commits into from Oct 16, 2021

Conversation

bpedersen
Copy link
Contributor

@bpedersen bpedersen commented Oct 13, 2021

Created Loop component for #254

  • Created exampled of looped in example package
  • Created tests
  • Created documentation in docs package

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


bpedersen and others added 6 commits October 13, 2021 13:02
* Created exampled of looped in example package
* Created tests
* Created documentation in docs package
@JonnyBurger
Copy link
Member

Thanks a lot! Looking very good! 😊 I started reviewing it, and making some changes.

IMO, the desired behavior for durationInFrames=20, loop=2 is that the component loops two times, then disappears. So I made this change, do you agree with it?

Another thing I changed is to disallow durationInFrames=Infinity. That would mean it never loops!

Also I used Sequences again, so we rely less on Internals. It also allows us to display it more nicely in the timeline. I'm adding a "x3" badge to the timeline if you use the Loop component and started implementing it.

Appreciate it so much that you added tests and docs, and an example. Makes it so much easier to process! 😊

I'm doing some more refinements tomorrow for the timeline and will then merge it. No actions are needed from your side, unless you have some inputs. Thanks again for an excellent PR 😃

@bpedersen
Copy link
Contributor Author

Yeah, I agree with the Infinity and the Sequence approach. It is more visual in the timeline of what is occuring and does reuse the logic from sequences instead of reimplementing it.
I would appreciate if you would tag this PR hacktoberfest-approved if you get a chance. Thanks

@JonnyBurger
Copy link
Member

😊 👍 Added label and merged! Very excited to ship it, thanks once again!

@JonnyBurger JonnyBurger merged commit 0391806 into remotion-dev:main Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants