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

Apply all non-valid props to the wrapper element #183

Merged
merged 1 commit into from
Apr 24, 2017
Merged

Conversation

cookpete
Copy link
Owner

Possible fix for #167

@oyeanuj any thoughts on this? The only possible issue is that we now call lodash's omit on every render, but I'm not sure it will affect performance that badly.

@oyeanuj
Copy link

oyeanuj commented Apr 17, 2017

Hey @cookpete! The lodash omit part looks good, I'd be surprised if it affects performance. I did have one question though - we are attaching the events/props to the div wrapper which can enclose multiple players (if I am correct in the understanding of how renderPlayers function is working.

I feel like for the original case I mentioned in #167, one would need these controls on individual players - like hovering on an individual player should only show that player's controls.

Thoughts?

@cookpete
Copy link
Owner Author

we are attaching the events/props to the div wrapper which can enclose multiple players (if I am correct in the understanding of how renderPlayers function is working.

Multiple players are only ever rendered when preload is set and youtube or vimeo players need to be "primed" by starting in the background. These players are never displayed so it doesn't make a difference.

@oyeanuj
Copy link

oyeanuj commented Apr 21, 2017

Oh, I see. Well, since they don't show up visually, then all of this looks good! Thanks so much!

@cookpete cookpete merged commit 1379b4f into master Apr 24, 2017
@cookpete cookpete deleted the rest-props branch April 24, 2017 19:45
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