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

Make it work under SSR #30

Merged
merged 4 commits into from
Oct 11, 2017
Merged

Make it work under SSR #30

merged 4 commits into from
Oct 11, 2017

Conversation

EyePulp
Copy link
Contributor

@EyePulp EyePulp commented Sep 28, 2017

Howdy;
I used some simple detection for global window & document objects to short-circuit attempts to make the video play when using server-side rendering under Vue.js 2.x

This is working for us currently, and tests are still passing. Let me know if you would like further changes or do not like this approach.

Also - I didn't bump the package.json, so that might have to happen if you accept the PR and want to bump the version and publish to NPM.

Thanks!

@kaorun343
Copy link
Owner

HI.

@EyePulp

Thanks for your PR.

I haven't tried SSR yet, but I understand its importance. 👍
I'm looking forward the changes by you. 😄

src/index.js Outdated
@@ -2,33 +2,35 @@ import { getIdFromURL, getTimeFromURL } from './utils'
import container from './container'
import YouTubePlayer from './player'

export { YouTubePlayer, getIdFromURL, getTimeFromURL }
export default { YouTubePlayer, getIdFromURL, getTimeFromURL,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the way of exporting.
This change is not necessary for the improvements, I think.

@EyePulp
Copy link
Contributor Author

EyePulp commented Oct 11, 2017

Hey @kaorun343, I moved it back to your original style on the export. I also rebuilt and bumped the nom version. Thanks for taking a look!

@kaorun343
Copy link
Owner

Thanks! I'll merge this.

@kaorun343 kaorun343 merged commit 01f85dc into kaorun343:master Oct 11, 2017
@EyePulp
Copy link
Contributor Author

EyePulp commented Oct 11, 2017

Thanks!

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