-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat(Egghead): Add support for Egghead #115
Conversation
Codecov Report
@@ Coverage Diff @@
## master #115 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 17 +1
Lines 215 221 +6
Branches 27 27
=========================================
+ Hits 215 221 +6
Continue to review full report at Codecov.
|
I've been looking at both the tests and the code and can't see what's wrong - I know this is starting to be a pattern sorry for that! The url is transformed as expected but not when the markdown file is opened. After comparing the code with the other transformers, I can't really see why the markdown test is failing, any idea? 🤔 |
* Solving MichaelDeBoey#97 * Update tests * Update src/transformers/Twitter.js (for the sake of consistency) Co-authored-by: Michaël De Boey <[email protected]> * Update src/__tests__/transformers/Twitter.js As far as we know, this can't be created Co-authored-by: Michaël De Boey <[email protected]> * Update src/__tests__/transformers/Twitter.js This too, is not possible. Co-authored-by: Michaël De Boey <[email protected]> * Update src/__tests__/transformers/__fixtures__/Twitter.md This too, is not possible. Co-authored-by: Michaël De Boey <[email protected]> Co-authored-by: Agastya Chandrakant <[email protected]> Co-authored-by: Michaël De Boey <[email protected]>
Hello Michael, well it took me way too long to finally attempt and fix the issue with the failing test. It's a bit silly but figured out that I had forgotten to add the transformer to index.js. I tried to pull the changes into the branch but something weird happened and for some reason I show up on the previous commits, not sure how to fix this. How do you want me to proceed here? Should I make a new branch and submit a new PR or it won't matter when you merge this PR? 🤔 Thanks for the time and patience with me on this PR and for having it open for such a long time! |
What: Added support for egghead videos/lessons
Why: Fixes #93
How: Added transformer that adds
/embed
to the end of the urlChecklist:
I'm still having issues with the last test on
Plugin can transform egghead links
not entire sure why this one is failing even thought theGet correct egghead iframe
is working. Will need to dig into this deeper to try and figure out why its happening.In regards to the transformer, should we add the title to the iframe? Should we add a
Video hosted on <a href={lessonLink}>egghead.io</a>
message under the video as well?