Fix youtube and embed links #14

cansavvy · 2021-08-25T17:52:44Z

Summary

This fixes two of the three things to fix mentioned on: #6 (comment)

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: #6 (comment)

Edit:

Fix three:

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

cansavvy · 2021-08-25T17:53:48Z

I also ran the code through styler

…eanbuild into cansavvy/link-splitters

cansavvy · 2021-08-25T17:57:08Z

R/replace_html.R

      na_empty(get_html_attr(x = x, name = name, element = element))
    })
-    names(out) = attributes
+    names(out) <- attributes


This is the brand new chunk of code that does the youtube link switching.

cansavvy · 2021-08-25T21:10:40Z

R/replace_html.R

+    'height: "{height}",',
+    'width: "{width}",',
+    'align: "{align}"',
+    'type: "{type}"',


I've added these two new attributes so we can specify the youtube poster image. 🎉

cansavvy · 2021-08-26T01:32:05Z

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 · 2021-08-26T01:36:32Z

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

cansavvy · 2021-08-26T01:37:44Z

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 · 2021-08-26T14:33:38Z

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 · 2021-08-26T14:59:38Z

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 · 2021-08-26T15:11:24Z

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 · 2021-08-26T15:14:13Z

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:

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

carriewright11 · 2021-08-26T15:20:32Z

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:

Checkout this branch

Run devtools::load_all("leanbuild") from your console

Switch to your cancer leadership course directory.

Run leanbuild::bookdown_to_leanpub()

Commit those changes on a new branch

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 · 2021-08-26T15:26:56Z

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 · 2021-08-26T15:31:21Z

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

Markua is fine with that for images.

carriewright11 · 2021-08-26T16:34:53Z

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

cansavvy · 2021-08-26T19:05:13Z

R/replace_html.R

+
+  ## Set defaults for items that haven't been specified
+
+  # Default for align is center


I've added these two defaults.

cansavvy · 2021-08-26T19:05:30Z

R/replace_html.R

+  }
+
+  # If its an image, use ! otherwise don't
+  if (!is.null(element) | element == "img") {


Adding logic to handle whether there should be a ! or not

This reverts commit 94579ac.

This reverts commit 62eea5f.

cansavvy · 2021-08-27T01:00:26Z

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?

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 · 2021-08-27T01:07:02Z

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 · 2021-08-27T12:03:00Z

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 · 2021-08-27T12:06:54Z

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"}

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 · 2021-08-27T12:13:08Z

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.

cansavvy added 4 commits August 25, 2021 07:43

Add in carriewright link splitting code

8e18cf6

Mapping things out

92ddc63

Styled code, youtube -> watch

4d8d022

Merge branch 'master' into cansavvy/link-splitters

dfcb9a0

cansavvy added 2 commits August 25, 2021 13:54

Get rid of old scratch script "split_embed_links.R"

5604a53

Merge branch 'cansavvy/link-splitters' of https://github.com/jhudsl/l…

004f8c2

…eanbuild into cansavvy/link-splitters

cansavvy commented Aug 25, 2021

View reviewed changes

cansavvy added 2 commits August 25, 2021 17:07

Adding youtube poster attribute

4dab4a7

Get rid of old version of fix

d173d50

cansavvy commented Aug 25, 2021

View reviewed changes

cansavvy changed the title ~~Make youtube links switch to "watch" link~~ Fix youtube links Aug 25, 2021

cansavvy added 4 commits August 25, 2021 21:05

add stringr as dependency

675d0ff

Add comma

7ad1b4e

Add link to include_url()

85c72ec

Change wording of links

874a24e

cansavvy changed the title ~~Fix youtube links~~ Fix youtube and embed links Aug 26, 2021

cansavvy mentioned this pull request Aug 26, 2021

simplest way to add the embedded files and video scripts although possibly there is a better way #3

Closed

Merge branch 'master' into cansavvy/link-splitters

74412a6

cansavvy requested a review from carriewright11 August 26, 2021 01:31

Get rid of stringr dependency

94579ac

Add logic for ! or not

7c50234

cansavvy commented Aug 26, 2021

View reviewed changes

cansavvy added 6 commits August 26, 2021 15:33

Fixing logic

91af811

Fix some of the logic

155b18a

Revert "Get rid of stringr dependency"

62eea5f

This reverts commit 94579ac.

Revert "Revert "Get rid of stringr dependency""

8641422

This reverts commit 62eea5f.

Auto stash before revert of "Revert "Get rid of stringr dependency""

53fb9bd

Fix logic around videos

7a05871

cansavvy mentioned this pull request Aug 27, 2021

Video expand buttons gone? Why? #17

Closed

cansavvy merged commit 9c5863e into master Aug 27, 2021

cansavvy mentioned this pull request Aug 27, 2021

Add commas and gha check #20

Merged

cansavvy deleted the cansavvy/link-splitters branch September 28, 2021 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix youtube and embed links #14

Fix youtube and embed links #14

cansavvy commented Aug 25, 2021 •

edited

Loading

cansavvy commented Aug 25, 2021

cansavvy Aug 25, 2021

cansavvy Aug 25, 2021

cansavvy commented Aug 26, 2021

cansavvy commented Aug 26, 2021

cansavvy commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

cansavvy commented Aug 26, 2021

cansavvy commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

cansavvy Aug 26, 2021

cansavvy Aug 26, 2021

cansavvy commented Aug 27, 2021

cansavvy commented Aug 27, 2021

carriewright11 commented Aug 27, 2021

carriewright11 commented Aug 27, 2021

cansavvy commented Aug 27, 2021


		## Set defaults for items that haven't been specified

		# Default for align is center

Fix youtube and embed links #14

Fix youtube and embed links #14

Conversation

cansavvy commented Aug 25, 2021 • edited Loading

Summary

Fix one:

Fix two:

Fix three:

cansavvy commented Aug 25, 2021

cansavvy Aug 25, 2021

Choose a reason for hiding this comment

cansavvy Aug 25, 2021

Choose a reason for hiding this comment

cansavvy commented Aug 26, 2021

cansavvy commented Aug 26, 2021

cansavvy commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

cansavvy commented Aug 26, 2021

cansavvy commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

carriewright11 commented Aug 26, 2021

cansavvy Aug 26, 2021

Choose a reason for hiding this comment

cansavvy Aug 26, 2021

Choose a reason for hiding this comment

cansavvy commented Aug 27, 2021

cansavvy commented Aug 27, 2021

carriewright11 commented Aug 27, 2021

carriewright11 commented Aug 27, 2021

cansavvy commented Aug 27, 2021

cansavvy commented Aug 25, 2021 •

edited

Loading