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

feat: make video embeddings optional. #2337

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

CheariX
Copy link
Contributor

@CheariX CheariX commented Apr 13, 2024

As discussed in #1181, I suggest to make embedding videos an optional feature.

This behavior aligns well with recently merged PR #2312.

Open questions:

  1. I added a youtube link to papers.bib. Is this link okay?
  2. I set enable_video_embedding: false as the default. I argue that privacy settings should be the default. Also, the current implementation of video.liquid only works for some very specific video URLs. For example, to embed youtube, specialized links must be used to avoid X-Frame-Option issues. This behavior can lead to a broken embedding, which would not look very nice.

Feedback welcome.

@alshedivat alshedivat requested a review from george-gca April 13, 2024 15:55
@george-gca
Copy link
Collaborator

I added a youtube link to papers.bib. Is this link okay?

Yes.

I argue that privacy settings should be the default.

You also mean the one that downloads the 3rd party libraries?

Also, the current implementation of video.liquid only works for some very specific video URLs. For example, to embed youtube, specialized links must be used to avoid X-Frame-Option issues. This behavior can lead to a broken embedding, which would not look very nice.

This PR solves this issue? If not, I believe this would be a nice addition to our README, maybe in FAQ.md?

Also, I believe the current solution (displaying the video below in a small collapsible area) for the bib part is not worth it. It is not even displaying properly, also the video is too small to be useful. I think we should change it to always open in a new window. What do you think?

_config.yml Outdated Show resolved Hide resolved
@CheariX
Copy link
Contributor Author

CheariX commented Apr 15, 2024

I argue that privacy settings should be the default.

You also mean the one that downloads the 3rd party libraries?

Yes. Basically, I'd like to change these settings in _config.yml:

enable_publication_badges:
  altmetric: false
  dimensions: false
  google_scholar: false

third_party_libraries:
  download: true

By implementing these settings, the website will not send requests to any third-party domains (a desirable feature in compliance with EU/GDPR laws, as there is no need for a consent banner). However, I have noticed one exception: https://distill.pub/third-party/katex/katex.min.js. Unfortunately, I could not find any means to disable this particular connection.

People who are fine with third party requests can then easily enable these features in the config.

Maybe we can open a PR or Issue to address and discuss this issue separately.

Also, the current implementation of video.liquid only works for some very specific video URLs. For example, to embed youtube, specialized links must be used to avoid X-Frame-Option issues. This behavior can lead to a broken embedding, which would not look very nice.

This PR solves this issue? If not, I believe this would be a nice addition to our README, maybe in FAQ.md?

  • video.liquid works for https://www.youtube.com/embed/aqz-KE-bpKQ => /embed/
  • video.liquid does not work for https://www.youtube.com/watch?v=aqz-KEP-bpKQ => /watch?v=
  • Please note that I used youtube-nocookie.com instead of youtube.com in paper.bib. But the result is the same for both domains: one must use the embed link to use youtube in an iframe.

I'm not sure what one should expect here.
I always had youtube links in my video tag, but no dedicated embedding links.
We could do some find/replace magic to allow regular links, but I did not want to touch video.liquid since I'm not gonna use the embedding feature anyway.

Also, I believe the current solution (displaying the video below in a small collapsible area) for the bib part is not worth it. It is not even displaying properly, also the video is too small to be useful. I think we should change it to always open in a new window. What do you think?

I'd be totally happy with that. If you like, I can add this removal to this PR.

Not sure about youtube, but I think one use case was that some people hosted their own *.mp4/webm/ogg files and refered them in the video tag

@george-gca
Copy link
Collaborator

People who are fine with third party requests can then easily enable these features in the config.

I would vote for the opposite, at least regarding the publication badges. It is useful to demonstrate how this would look like in our demo site, in case the user wants to keep it. Maybe add something citing how/why to disable this regarding privacy issues, probably including some of the information already cited here, in one of our README files, maybe CUSTOMIZE.md.

I'd be totally happy with that. If you like, I can add this removal to this PR.

Not sure about youtube, but I think one use case was that some people hosted their own *.mp4/webm/ogg files and refered them in the video tag

I think this addition would be nice to have. Even though someone might embed their own videos there, the viewport is too small to be useful (I think). If the user wants, it would be more useful to open it in a new tab, or even open it in a way similar to a zoomed image works.

@CheariX
Copy link
Contributor Author

CheariX commented Apr 15, 2024

I think this addition would be nice to have. Even though someone might embed their own videos there, the viewport is too small to be useful (I think). If the user wants, it would be more useful to open it in a new tab, or even open it in a way similar to a zoomed image works.

What kind of addition do you mean?
Do you want any further changes in this PR?

The embedding feature can be enabled with the enable_video_embedding: true option.

Edit: Maybe I should solve the merge conflict first...

@CheariX CheariX requested a review from george-gca April 15, 2024 18:49
@george-gca
Copy link
Collaborator

Also, I believe the current solution (displaying the video below in a small collapsible area) for the bib part is not worth it. It is not even displaying properly, also the video is too small to be useful. I think we should change it to always open in a new window. What do you think?

I'd be totally happy with that. If you like, I can add this removal to this PR.

I was talking about this part.

@george-gca
Copy link
Collaborator

Thanks for the changes.

@george-gca george-gca merged commit a03b2e7 into alshedivat:master Apr 15, 2024
3 checks passed
george-gca pushed a commit to george-gca/multi-language-al-folio that referenced this pull request Apr 15, 2024
As discussed in alshedivat#1181, I suggest to make embedding videos an optional
feature.

This behavior aligns well with recently merged PR alshedivat#2312.

Open questions:

1. I added a youtube link to `papers.bib`. Is this link okay?
2. I set `enable_video_embedding: false` as the default. I argue that
privacy settings should be the default. Also, the current implementation
of `video.liquid` only works for some very specific video URLs. For
example, to embed youtube, specialized links must be used to avoid
`X-Frame-Option` issues. This behavior can lead to a broken embedding,
which would not look very nice.

Feedback welcome.
@CheariX CheariX deleted the feat-optional-video-embedding branch April 15, 2024 21:11
BoAi01 pushed a commit to BoAi01/boai01.github.io that referenced this pull request May 7, 2024
As discussed in alshedivat#1181, I suggest to make embedding videos an optional
feature.

This behavior aligns well with recently merged PR alshedivat#2312.

Open questions:

1. I added a youtube link to `papers.bib`. Is this link okay?
2. I set `enable_video_embedding: false` as the default. I argue that
privacy settings should be the default. Also, the current implementation
of `video.liquid` only works for some very specific video URLs. For
example, to embed youtube, specialized links must be used to avoid
`X-Frame-Option` issues. This behavior can lead to a broken embedding,
which would not look very nice.

Feedback welcome.
siril-teja pushed a commit to siril-teja/siril-teja.github.io-old that referenced this pull request Jun 19, 2024
As discussed in alshedivat#1181, I suggest to make embedding videos an optional
feature.

This behavior aligns well with recently merged PR alshedivat#2312.

Open questions:

1. I added a youtube link to `papers.bib`. Is this link okay?
2. I set `enable_video_embedding: false` as the default. I argue that
privacy settings should be the default. Also, the current implementation
of `video.liquid` only works for some very specific video URLs. For
example, to embed youtube, specialized links must be used to avoid
`X-Frame-Option` issues. This behavior can lead to a broken embedding,
which would not look very nice.

Feedback welcome.
karapostK pushed a commit to karapostK/karapostK.github.io that referenced this pull request Jul 4, 2024
As discussed in alshedivat#1181, I suggest to make embedding videos an optional
feature.

This behavior aligns well with recently merged PR alshedivat#2312.

Open questions:

1. I added a youtube link to `papers.bib`. Is this link okay?
2. I set `enable_video_embedding: false` as the default. I argue that
privacy settings should be the default. Also, the current implementation
of `video.liquid` only works for some very specific video URLs. For
example, to embed youtube, specialized links must be used to avoid
`X-Frame-Option` issues. This behavior can lead to a broken embedding,
which would not look very nice.

Feedback welcome.
Suraj-Bhor pushed a commit to Suraj-Bhor/suraj-bhor.github.io that referenced this pull request Aug 13, 2024
As discussed in alshedivat#1181, I suggest to make embedding videos an optional
feature.

This behavior aligns well with recently merged PR alshedivat#2312.

Open questions:

1. I added a youtube link to `papers.bib`. Is this link okay?
2. I set `enable_video_embedding: false` as the default. I argue that
privacy settings should be the default. Also, the current implementation
of `video.liquid` only works for some very specific video URLs. For
example, to embed youtube, specialized links must be used to avoid
`X-Frame-Option` issues. This behavior can lead to a broken embedding,
which would not look very nice.

Feedback welcome.
meiqing-wang pushed a commit to meiqing-wang/meiqing-wang.github.io that referenced this pull request Oct 13, 2024
As discussed in alshedivat#1181, I suggest to make embedding videos an optional
feature.

This behavior aligns well with recently merged PR alshedivat#2312.

Open questions:

1. I added a youtube link to `papers.bib`. Is this link okay?
2. I set `enable_video_embedding: false` as the default. I argue that
privacy settings should be the default. Also, the current implementation
of `video.liquid` only works for some very specific video URLs. For
example, to embed youtube, specialized links must be used to avoid
`X-Frame-Option` issues. This behavior can lead to a broken embedding,
which would not look very nice.

Feedback welcome.
avishekanand pushed a commit to avishekanand/al-folio-homepage that referenced this pull request Oct 22, 2024
As discussed in alshedivat#1181, I suggest to make embedding videos an optional
feature.

This behavior aligns well with recently merged PR alshedivat#2312.

Open questions:

1. I added a youtube link to `papers.bib`. Is this link okay?
2. I set `enable_video_embedding: false` as the default. I argue that
privacy settings should be the default. Also, the current implementation
of `video.liquid` only works for some very specific video URLs. For
example, to embed youtube, specialized links must be used to avoid
`X-Frame-Option` issues. This behavior can lead to a broken embedding,
which would not look very nice.

Feedback welcome.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants