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(docs): allow audio (mp3/ogg), video (mp4/webm) and font (woff2) attachments #7605

Merged
merged 26 commits into from
Jun 2, 2023

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Nov 16, 2022

Summary

Fixes #5727.

Problem

So far, only image files could be put next to page files (index.md) to be used in live samples, but some examples require media files or fonts.

Solution

Extend the current approach from images to file attachments in general, including audio/video/font files.


Screenshots

n/a


How did you test this change?

  1. Checked out this branch.
  2. Opened http://localhost:3000/en-US/docs/Web/HTML/Element/audio#specifying_a_single_source locally.
  3. Verified that the audio plays.

Note: Didn't test video files nor fonts yet.

@caugner caugner force-pushed the 5727-allow-audio-video-font-attachments branch from 363ccc8 to 26689b5 Compare November 16, 2022 20:12
@github-actions github-actions bot added the flaw-system issues and feature requests related to the flaws system label Nov 16, 2022
@caugner
Copy link
Contributor Author

caugner commented Nov 16, 2022

@hamishwillee @wbamberg Would you be able to test if this covers your use cases? And whether the added file extensions (mp3, mp4, ttf, webm) suffice?

@wbamberg
Copy link
Collaborator

wbamberg commented Nov 16, 2022

This is great @caugner !

I tested this with local mp3 and local ttf, and pushed the changes to https://github.com/wbamberg/content/tree/local-fonts (changes):

The audio file works great but the font doesn't load. Am I doing something wrong? Is it different because the resource is loaded from CSS?

@caugner
Copy link
Contributor Author

caugner commented Nov 17, 2022

The audio file works great but the font doesn't load. Am I doing something wrong? Is it different because the resource is loaded from CSS?

Sorry, now it should work:
image

@caugner caugner added the wx writer experience label Nov 17, 2022
@caugner caugner marked this pull request as ready for review November 17, 2022 12:17
content/file-attachment.ts Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Contributor

@caugner @wbamberg MP4 video works great - I tested local mp4 by replacing the video element (http://localhost:3000/en-US/docs/Web/HTML/Element/video#examples) with a downloaded copy.

But I didn't upload a test branch, because that file is 60MB.

Which brings me to another point; if we're going to allow these files, we probably need to agree when you should use a local file or not, and also perhaps limit the file size. Every file we add increases the cost of syncing to github - so IMO we shouldn't use this by default - only when they are needed to demonstrate something.

Happy to test woff2 when it is supported.

@wbamberg
Copy link
Collaborator

@caugner @wbamberg MP4 video works great - I tested local mp4 by replacing the video element (http://localhost:3000/en-US/docs/Web/HTML/Element/video#examples) with a downloaded copy.

But I didn't upload a test branch, because that file is 60MB.

Which brings me to another point; if we're going to allow these files, we probably need to agree when you should use a local file or not, and also perhaps limit the file size. Every file we add increases the cost of syncing to github - so IMO we shouldn't use this by default - only when they are needed to demonstrate something.

Yes, we should have guidance for this.

@Rumyra was talking the other day about how it would be good to have an "asset library" for things like images, audio, video, fonts. There are benefits to this:

In theory that could mean we don't need local assets - because we can get the proper version of MonteCarlo.ttf and add it to the library, and refer to it there (if assets from the library are served).

Still I think there's a bit of a tension about local assets because it's nice (for authors) to keep assets with the page, but yes there is a cost in terms of repo size, and maybe it's not worth it always. I think it would be worth doing some math here :).

But all that's a further question, for the time being it's great to have this working and means we can have better font docs, which is important for interop 2022 docs: openwebdocs/project#137.

@hamishwillee
Copy link
Contributor

Still I think there's a bit of a tension about local assets because it's nice (for authors) to keep assets with the page, but yes there is a cost in terms of repo size, and maybe it's not worth it always. I think it would be worth doing some math here :).

Yes. IMO the biggest benefit of a shared asset folder is to be able to avoid having to copy the image for translation. You can still do so if you want and have a local copy, but in my experience most translators do not bother to translate images.

But all that's a further question, for the time being it's great to have this working ...

Too right. This would have saved me a heap of time in the past, and I am sure will do so again.

@caugner caugner marked this pull request as draft November 18, 2022 09:27
@caugner caugner force-pushed the 5727-allow-audio-video-font-attachments branch from 04d6f72 to 876efe2 Compare November 18, 2022 19:11
Copy link
Contributor Author

@caugner caugner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to add checks to the filecheck/checker.

@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label Nov 25, 2022
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@caugner caugner force-pushed the 5727-allow-audio-video-font-attachments branch from 876efe2 to b75f210 Compare November 25, 2022 19:16
@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Nov 25, 2022
@hamishwillee
Copy link
Contributor

@caugner Let me know if this is waiting on me for testing or anything (I don't think so, just don't want to be a blocker.)

build/cli.ts Outdated Show resolved Hide resolved
@caugner caugner requested a review from fiji-flo November 28, 2022 11:28
@caugner caugner force-pushed the 5727-allow-audio-video-font-attachments branch 4 times, most recently from bf748cb to a6a0df6 Compare November 28, 2022 20:49
@caugner caugner marked this pull request as ready for review November 28, 2022 20:57
@caugner caugner changed the title chore(build,content,server): allow mp3/mp4/ogg/ttf/webm attachments chore(build,content,server): allow mp3/mp4/ogg/webm/woff2 attachments Nov 29, 2022
build/index.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
filecheck/checker.ts Outdated Show resolved Hide resolved
@caugner caugner force-pushed the 5727-allow-audio-video-font-attachments branch from 1cec4c7 to c22d47d Compare May 19, 2023 17:45
@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label May 19, 2023
@dletorey
Copy link
Contributor

I have run into an issue while trying to run filecheck on an avif image.

yarn filecheck files/en-us/web/css/content/mdn-1.avif                         1 ↵
yarn run v1.22.19
$ env-cmd --silent cross-env CONTENT_ROOT=files yari-filecheck --cwd=. files/en-us/web/css/content/mdn-1.avif
progress [========================================] 100% | ETA: 0s | 1/1

error: Error: files/en-us/web/css/content/mdn-1.avif has an unrecognized mime type: image/avif

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Is there a plan to add image/avif?

@hamishwillee
Copy link
Contributor

Ah - @caugner So if we add avif we should also update filecheck, at least for images to at least say "not compressible by us".

@dletorey
Copy link
Contributor

If we do decide to add image/avif file types can we also add image/webp at the same type.
I have just got a build error for webp too.

Run echo files/en-us/web/css/content/mdn.png files/en-us/web/css/content/mdn.webp
files/en-us/web/css/content/mdn.png files/en-us/web/css/content/mdn.webp
yarn run v1.22.19
$ env-cmd --silent cross-env CONTENT_ROOT=files yari-filecheck --cwd=. files/en-us/web/css/content/mdn.png files/en-us/web/css/content/mdn.webp

error: Error: files/en-us/web/css/content/mdn.webp has an unrecognized mime type: image/webp

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 1.

@caugner
Copy link
Contributor Author

caugner commented May 25, 2023

@dletorey @hamishwillee We will not be adding AVIF or WebP support as part of this PR, but if there are good reasons to add those image types separately, I don't see why we wouldn't.

PS: I reverted the change that added AVIF support in this PR, as I mistook AVIF for a video format. 🤦

@hamishwillee
Copy link
Contributor

hamishwillee commented May 26, 2023

@caugner Thank you for the information. I'm keen we get this in for the use case that matter now, so this is good.

are good reasons to add those image types separately, I don't see why we wouldn't.

FWIW I don't think there is a good reason to do so right now, based on this evaluation.

Generally you choose the compression that gives you the smallest file size for the quality you need. If we had lots of photographs then that would be avif or webp. But most of the content on MDN is text/line drawings for which PNG does much better. I tend to use SVG if possible because it scales well and if I have to make changes later, my content format is also a format I can easily edit. Otherwise PNG. JPG often ends up being a little grainy.

That said, support for these would be a "nice to have."

@caugner
Copy link
Contributor Author

caugner commented May 29, 2023

@fiji-flo Tested locally with a local .mp4 file on http://localhost:3000/en-US/docs/Web/HTML/Element/video, and worked for me. Can you give it another try?

@dletorey
Copy link
Contributor

@caugner

if there are good reasons to add those image types separately, I don't see why we wouldn't.

The reason that this came up for me is that I was trying to create an example of switching between different image types using image-set()

.selector {
  content: image-set(
      "https://assets.codepen.io/5342839/mdn.avif" type("image/avif"),
      "https://assets.codepen.io/5342839/mdn.png" type("image/png")
   );
}

Which highlighted the fact to me that avif were not supported by Yari.

@caugner
Copy link
Contributor Author

caugner commented May 31, 2023

The reason that this came up for me is that I was trying to create an example of switching between different image types using image-set()

@dletorey Curious: Is there a browser that supports image-set(), but doesn't support avif?

@dletorey
Copy link
Contributor

Is there a browser that supports image-set(), but doesn't support avif?

Image-set has been around in numerous browsers for quite sometime, while avif is kinda newer.

The issue I was having was with adding an avif image and yari throwing error as the type is not supported

error: Error: files/en-us/web/css/content/mdn-1.avif has an unrecognized mime type: image/avif

Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caugner I small thing otherwise this looks good. Deploying it to stage now (as part of the next branch)

libs/constants/index.js Show resolved Hide resolved
@caugner caugner changed the title feat(docs): allow audio (mp3/ogg), video (mp4/webm) and font (woff2) assets feat(docs): allow audio (mp3/ogg), video (mp4/webm) and font (woff2) attachments Jun 2, 2023
@caugner caugner merged commit 73f8dbc into main Jun 2, 2023
@caugner caugner deleted the 5727-allow-audio-video-font-attachments branch June 2, 2023 22:26
@wbamberg
Copy link
Collaborator

wbamberg commented Jun 7, 2023

Thank you for adding this feature! It should be really useful.

I was just looking at using it to fix mdn/content#21597 properly, but noticed it only supports woff, not ttf. Is there a reason for that, and would it be possible to add ttf support? It seems like I might be able to convert the ttf font to woff, but I'm not sure if that's the best option.

@caugner
Copy link
Contributor Author

caugner commented Jun 8, 2023

I was just looking at using it to fix mdn/content#21597 properly, but noticed it only supports woff, not ttf.

@wbamberg I removed TTF support from the PR when I noticed how much bigger TTF files seem to be compared to woff2. Could we just use the woff2 version of Monte Carlo for that example instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaw-system issues and feature requests related to the flaws system github-actions internal wx writer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using relative URLs to refer to audio/video/font files
8 participants