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

adding Show/Hide Player function #9

Closed
wants to merge 1 commit into from

Conversation

shershen08
Copy link

#5

@shershen08
Copy link
Author

@sdras
Copy link
Owner

sdras commented Mar 8, 2017

Thank you for the PR! Though I like the ability to show and hide the player, I won't merge this, but here's why (so you know I'm not rejecting this for no reason):

  • It snaps in and out of view- I don't think a setTimeout is appropriate for this, why not use GreenSock to animate since it's a dependency? It should have a smoother transition
  • I would like the user to have a bit more control over showing and hiding, something like a tab or a button that could expand and collapse- removing it on hover is a little jarring and unexpected, I'd like the user to be in full control over it and know why and when this happens
  • This wouldn't work on mobile.

All that said, I think the concept is a good one! I like how the mo.js player does it: http://codepen.io/sdras/pen/6774e07346aed8977e87f09aa282c982 (that said, we'd need a slightly different design concept because this player isn't necessarily flush with the bottom of the screen). Thanks!

@sdras sdras closed this Mar 8, 2017
@shershen08
Copy link
Author

Thanks for detailed explanation 👍

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.

None yet

2 participants