search

jhudsl / ottrpal

Tools for converting OTTR courses into Leanpub or Coursera courses :otter:
https://jhudatascience.org/ottrpal/
GNU General Public License v3.0
3 stars 1 forks source link

Fix youtube and embed links #14

Closed cansavvy closed 3 years ago

cansavvy commented 3 years ago

Summary

This fixes two of the three things to fix mentioned on: https://github.com/jhudsl/leanbuild/issues/6#issuecomment-905746970

Fix one:

Markua needs the "watch" version of youtube links. So where we have a link like: "https://www.youtube.com/embed/" we actually need: "https://www.youtube.com/watch?v="

I've added this change to the existing replace_image_data() function of this package. This function appears to parse out all the html attributes so its the perfect time to fix youtube links.

This works when I've run it locally.

Fix two:

I added the type and poster specification for youtube links as prescribed by @carriewright11 here: https://github.com/jhudsl/leanbuild/issues/6#issuecomment-902849601

Edit:

Fix three:

I believe I've made the changes needed for adding the links for instances where knitr::include_url() are used.

cansavvy commented 3 years ago

I also ran the code through styler

cansavvy commented 3 years ago

I believe this closes #6 and makes leanbuild that much more functional. @carriewright11 do you want to see if this does what you need?

cansavvy commented 3 years ago

Oh great. 😩 Windows fails because it wants a Git token to install a dependency. I'll have to fix that.

cansavvy commented 3 years ago

The lazy/efficient way around this problem is to use functions that aren't part of stringr so we don't have that dependency. I will probably do this.

carriewright11 commented 3 years ago

I believe this closes #6 and makes leanbuild that much more functional. @carriewright11 do you want to see if this does what you need?

Taking a look now.

carriewright11 commented 3 years ago

Great work! Yeah, looks good. I should test on my files (although maybe you did that?) Can we add instructions to the video conversions - so people know how to expand the videos in Leanpub?(maybe I missed that though).

Does the exclamation mark need to move to the end here: x <- paste0(x, paste0("Check out this ![link](", myenv$src, ")."))

cansavvy commented 3 years ago

Great work! Yeah, looks good. I should test on my files (although maybe you did that?)

I was using the 05-promoting_diversity.Rmd file from your course to test and it looked like what it should be as far as my knowledge but I didn't have the ability to push to Leanpub for the final test.

Can we add instructions to the video conversions - so people know how to expand the videos in Leanpub?(maybe I missed that though).

Sounds good!

Does the exclamation mark need to move to the end here: x <- paste0(x, paste0("Check out this ![link](", myenv$src, ")."))

The exclamation there is a part of the link but we can certainly make the sentence itself exclamatory. Should there not be a ! in front of the brackets? Perhaps Markua doesn't like that?

cansavvy commented 3 years ago

If you would be able to test on Leanpub for real, that'd be great. If you want to do a full test of this before we merge: 1) Checkout this branch 2) Run devtools::load_all("leanbuild") from your console 3) Switch to your cancer leadership course directory. 4) Run leanbuild::bookdown_to_leanpub() 5) Commit those changes on a new branch 6) Preview on leanpub but switch to the new branch you have.

carriewright11 commented 3 years ago

If you would be able to test on Leanpub for real, that'd be great. If you want to do a full test of this before we merge:

  1. Checkout this branch
  2. Run devtools::load_all("leanbuild") from your console
  3. Switch to your cancer leadership course directory.
  4. Run leanbuild::bookdown_to_leanpub()
  5. Commit those changes on a new branch
  6. Preview on leanpub but switch to the new branch you have.

I actually published my course yesterday... so I'd need to look into how that works... if you can preview without modifying the published course. Otherwise we could use the template?

carriewright11 commented 3 years ago

The exclamation there is a part of the link but we can certainly make the sentence itself exclamatory. Should there not be a ! in front of the brackets? Perhaps Markua doesn't like that?

Since these aren't image links, we wouldn't want the ! though right? or maybe I am missing something. And yeah I kinda like the sentence being exclamatory :P but it depends on people's writing styles I suppose.

carriewright11 commented 3 years ago

Should there not be a ! in front of the brackets? Perhaps Markua doesn't like that?

Markua is fine with that for images.

carriewright11 commented 3 years ago

OK I am testing from the template... You can see the test now when you look at the latest preview version on leanpub

cansavvy commented 3 years ago

This is working as far as I can tell, but the one thing that's odd/not working really is the corner part in the videos are gone?

Screen Shot 2021-08-26 at 8 24 20 PM

This is what the tags for the above look like (also note that "middle" doesn't work -- only "center"). 

{height: "400px", width: "672", align: "middle" type: "video" poster: "http://img.youtube.com/vi/VOCYL-FNbr0/mqdefault.jpg"}
![Click on the lower right corner to expand the screen](https://www.youtube.com/watch?v=VOCYL-FNbr0).
{height: "400px", width: "672", align: "middle" type: "video" poster: "http://img.youtube.com/vi/VOCYL-FNbr0/mqdefault.jpg"}
![Click on the lower right corner to expand the screen](https://www.youtube.com/watch?v=VOCYL-FNbr0).
cansavvy commented 3 years ago

This PR has gotten a bit out of hand. I'm going to merge it since it is fixing things and then file separate issues for these new things popping up.

carriewright11 commented 3 years ago

This is working as far as I can tell, but the one thing that's odd/not working really is the corner part in the videos are gone?

They aren't gone... it shows up after pressing play... but idk how to convey that more concisely/ assumed people when they saw the word video would press play. They can actually press twice on the right corner (on nothing) and it will expand- you don't have to click the center to play.

The text appears to be centered but not the video... let me check how I did that...

carriewright11 commented 3 years ago

This is working as far as I can tell, but the one thing that's odd/not working really is the corner part in the videos are gone?

They aren't gone... it shows up after pressing play... but idk how to convey that more concisely/ assumed people when they saw the word video would press play. They can actually press twice on the right corner (on nothing) and it will expand- you don't have to click the center to play.

The text appears to be centered but not the video... let me check how I did that...

Ok here is an example of mine:

{type: video, poster: 'http://img.youtube.com/vi/VOCYL-FNbr0/mqdefault.jpg',height: "400px", width: "100%", align: "middle"} Click lower right corner of video to expand

I noticed that the comma is missing before align:"middle:" on the code so maybe that is why? Also I only tested with type:video, poster: at the beginning. Idk but maybe that makes a difference.

cansavvy commented 3 years ago

I noticed that the comma is missing before align:"middle:" on the code so maybe that is why? Also I only tested with type:video, poster: at the beginning. Idk but maybe that makes a difference.

Perhaps. It's also odd the comma isn't showing up. I'll look into that.