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 nbVideo & nbAudio (revive #153) #244

Merged
merged 3 commits into from
Jul 10, 2024
Merged

Conversation

neroist
Copy link
Contributor

@neroist neroist commented Jun 26, 2024

for all_blocks.nim I wasn't sure what to put, so I ended up just putting Bad Apple instead :P please feel free to change! maybe reference some of that stuff in the Q&A. or maybe you should keep it instead, perhaps.

Some changes made:

  • relToRoot uses isAbsolute -- thought this was better than checking for "http" as the first four letters of the string
  • You can now set the MIME type of the media (doesn't assume based on file ext.), and set the autoplay, muted, and loop attributes of the video/audio html tag

@pietroppeter
Copy link
Owner

thanks! the changes greatly improve on #153 :)

I just learned about bad apple yesterday and I am definitely very amused 🤣 by this coincidence 🤯 and approve of the choice 🍎.

Instead of references in Q&A we could:

  • add a link to this explainer as the example url for nbVideo element (two advantages: 1) we show the url can be both relative and a web url; 2) we can remove the 7MB video asset)
  • invert nbAudio and nbVideo section in allblocks.nim (so that the audio comes before the "explainer")

Last thing is to ask if @HugoGranstrom did know about bad apple (he is definitely younger than me) and if he is ok with the choice.

@HugoGranstrom
Copy link
Collaborator

Had no idea about Bad Apple, but I definitely applaude the choice 😂

@neroist
Copy link
Contributor Author

neroist commented Jul 3, 2024

add a link to this explainer as the example url for nbVideo element

afaik, its impossible to insert a Youtube video into a <video> tag without some external Javascript dependency. We should probably add a link to the explainer in the nbVideo section instead.

For the audio & video assets, those can be replaced by either of these two from archive.org.

@pietroppeter
Copy link
Owner

Ah I guess you are right. This poses the question, should nbVideo be just a little wrapper for html video tag or also do the right thing (to embed a YouTube video we could use a iframe instead of JS right?) in case eg you put a link to YouTube?

I do not think we should get an answer in this PR, we could open a new issue if we feel like discussing this.

For the scope of the PR I would just add a link to the explainer for the moment. I think assets of a few MB are fine and there is no need to go with archive (which I guess might be slower to be reached? Or maybe not?)

@neroist
Copy link
Contributor Author

neroist commented Jul 8, 2024

sorry it took so long! should be ready to merge, no?

@pietroppeter pietroppeter merged commit 4af79ac into pietroppeter:main Jul 10, 2024
7 checks passed
@pietroppeter
Copy link
Owner

yep, looks good to me, thanks!

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.

3 participants