Conversation
|
wow, thanks! Cool feature. I'll need some time to review it. Hopefully I'll get around to it today |
src/compiler/GardenPageCompiler.ts
Outdated
| /!\[\[(.*?)(\.(png|jpg|jpeg|gif|webp))\|(.*?)\]\]|!\[\[(.*?)(\.(png|jpg|jpeg|gif|webp))\]\]/g; | ||
| const transcludedImageMatches = text.match(transcludedImageRegex); | ||
| const transcludedMediaItemRegex = | ||
| /!\[\[(.*?)(\.(png|jpg|jpeg|gif|webp|mp3|wav|ogg|m4a))\|(.*?)\]\]|!\[\[(.*?)(\.(png|jpg|jpeg|gif|webp|mp3))\]\]/g; |
There was a problem hiding this comment.
This seems fishy. The regex looks for mp3|wav|ogg|m4a in the first part, but only mp3 in the second part.
Also the code below always uses
const isAudio = linkedFile.extension === "mp3";
(i.e. only checks for mp3 files)
Except when deleting, where it checks for multiple file extensions again.
I feel like it would be a better idea to split image and audio file handling into separate code paths (using a distinct regex for each) and to avoid the isAudio checks down the line. Checking for specific file extensions at multiple points is asking for trouble when the set of file extensions changes at some point.
Maybe it's possible to attach kind of a "tag" to the Asset object denoting which type of asset it is? Second best would be having an isAudio function which checks for the correct extensions.
|
Thanks for you work. I had a quick look at it and left a comment on a part that caught my eye. I am interested to get video embeds working (#80) and this seems like a good starting point to piggy back off of. |
|
Thanks for the feedback! I'm putting my hands on it again. |
This reverts commit 36f39bd.
|
Still pending TODOs:
|
|
@oleeskild @foxblock ready to review again :) |
|
interesting feature! |
4 participants
These changes process also audio files. Images then turn into
<img>HTML tags and audios into<audio>HTML tags.It tries to solve #581.
I tested it manually and it works as expected.
However it also needs changes done in
.eleventy.jsso it knows how to process the audio files, so this is done into another PR: oleeskild/digitalgarden#306