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

Add support for twitter video embeds. #30

Merged
merged 3 commits into from
Jun 18, 2016
Merged

Conversation

mikelax
Copy link
Contributor

@mikelax mikelax commented Jun 13, 2016

Addresses issue #24

@orrybaram
Copy link
Contributor

@iefserge and @kesla can chime in here, but I think we're sticking with deku ^1.0.0 for now, which is why you might have been getting these element errors.

@nazar
Copy link
Contributor

nazar commented Jun 14, 2016

hey @orrybaram - not sure why the tests failed specifically on node 4 only. npm test passed on my host (v4.4.3) and also on travis for node 4 and 0.10

@nazar
Copy link
Contributor

nazar commented Jun 14, 2016

but I think we're sticking with deku ^1.0.0 for now,

deku ^1.0.0 was under devDependencies and I added "deku": "^2.0.0-rc16" under dependencies to match the deku version in article-json-to-fbia (as that package had issues when deku attempted to render virtual-element elements) where I notice that the embeds package is held back to ^1.4.2.

I ran npm test under v4.4.5 (to match travis) on my dev and all tests pass. Would it be possible to re-run the travis tests again? I had a quick look but was not able to.

@nazar
Copy link
Contributor

nazar commented Jun 14, 2016

This PR is required by html-to-article-json#58 and article-json-to-fbia#4

@nazar
Copy link
Contributor

nazar commented Jun 16, 2016

Hey @iefserge and @kesla,

Any updates on the status of this PR? I'd be more than happy to offer any clarifications or further commits to address any concerns you might have.

Thanks!

@iefserge
Copy link
Contributor

@nazar I agree, I think we should keep using deku@1 for now. We can upgrade later to the stable deku@2 when available, but it's going to be a major release.

@nazar
Copy link
Contributor

nazar commented Jun 16, 2016

Thanks for the update @iefserge - as far as I can tell, doku@1 is only required for the tests.

The bigger issue I think is the removal of virtual-element which didn't work when embeds@2 is being used by article-json-to-fbia which itself uses deku@2 but an older version of embeds@^1.4.2

Should we have branched off embeds@1.5.0 instead for adding the twitter-video support for article-json-to-fbia? I'll also try and revert back to deku@1

@kesla
Copy link
Contributor

kesla commented Jun 16, 2016

Hey,

We should merge micnews/article-json-to-fbia#2 to move article-json-to-fbia to deku@1 then we can merge this as well (with deku@1 support).

@nazar sorry for the confusion

@nazar
Copy link
Contributor

nazar commented Jun 16, 2016

Thanks @kesla - that makes a lot of sense now :D - I'm moving this to deku@1

@nazar
Copy link
Contributor

nazar commented Jun 16, 2016

Hey @kesla - reverted to use deku@1 and magic-virtual-element

@kesla kesla merged commit 64a8a26 into micnews:master Jun 18, 2016
@kesla
Copy link
Contributor

kesla commented Jun 18, 2016

@nazar ty

@kesla
Copy link
Contributor

kesla commented Jun 18, 2016

@nazar released in 2.5.0!

@mikelax
Copy link
Contributor Author

mikelax commented Jun 18, 2016

Thanks a lot @kesla.

@kesla
Copy link
Contributor

kesla commented Jun 18, 2016

@mikelax No, thank you. This is a feature that we'll definitely will have good use of ourselves!

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.

5 participants