search

benwiley4000 / cassette

📼 A flexible media player component library for React that requires no up-front config
https://benwiley4000.github.io/cassette/styleguide
MIT License
185 stars 28 forks source link

Enable injection of media element into PlayerContextProvider #349

Closed iantbutler01 closed 5 years ago

iantbutler01 commented 5 years ago

So it was pretty simple to enable. After I added it to the context I used the ExamplePlayerContextProvider.js to test that changing the prop to a function that returns null breaks the component like it should, that not including the prop uses the default I set and that injecting another element works as it should.

I wasn't sure what to name the prop so I went with createMediaElement, let me know what you think of that.

Happy to make changes to fit with what you're thinking.

iantbutler01 commented 5 years ago

Also the base branch here might be incorrect, I wasn't certain of that. The project was just on the "next" branch locally so I went with it, I can move it onto a branch based off master if that's not correct.

benwiley4000 commented 5 years ago

next is correct, unless you wanted to implement this for the version in master (master is much different/simpler). These days I don't add new features much.

I'll take a look at this soon - thank you!

iantbutler01 commented 5 years ago

Sounds good, I also didn't see a place for unit tests and the like, even though I went through it in the example stuff and did a quick does this work check. Let me know if you usually do anything besides that and I'll make it happen.

benwiley4000 commented 5 years ago

I have one snapshot test and no other tests at the moment. This is something I've been looking for time to work on. If you have any tests you'd like to write with Jest I'd be happy to have them (you can stick them in a __tests__ subdirectory like this and jest will pick them up automatically: https://github.com/benwiley4000/cassette/tree/next/packages/player/src/controls/__tests__).

benwiley4000 commented 5 years ago

I should add, don't kill yourself to figure that out. I don't have phantomjs or jsdom or anything set up so I'm not sure what the best way would be to test this feature.

iantbutler01 commented 5 years ago

Oh yeah, that makes it a bit more complicated :P Well I'll poke at it a bit see what happens

iantbutler01 commented 5 years ago

So spent some time looking into it and without properly mounting this on a dom it doesn't look useful to test. I could do a snapshot but, knowing that it renders with the specific props doesn't seem particularly useful as compared to knowing the audio element is properly passed through and that the various events can trigger properly on that passed in element.

iantbutler01 commented 5 years ago

Glad to hear it :)

benwiley4000 commented 5 years ago

Released in v2.0.0-alpha.22.

iantbutler01 commented 5 years ago

woot woot