FHachez / obsidian-convert-url-to-iframe

Plugin for Obsidian.md to convert a selected URL to an iframe.
200 stars 17 forks source link

Simplifying the output: managing aspect ratio with css #30

Closed FHachez closed 2 years ago

FHachez commented 2 years ago

Currently the output is not clean and simple:

<div style="display: block; position: relative; width: 100%; height: 0px; --aspect-ratio:9/16; padding-bottom: calc(var(--aspect-ratio) * 100%);">
  <iframe src="https://www.youtube.com/embed/mSC6GwizOag" allow="fullscreen" style="position: absolute; top: 0px; left: 0px; height: 100%; width: 100%;"></iframe>
</div>

It could simply be:

<iframe src="https://www.youtube.com/embed/mSC6GwizOag" allow="fullscreen" style="aspect-ratio:16/9; object-fit:contain; width: 100%;"></iframe>
kankaristo commented 2 years ago

The latter version doesn't seem to give the correct height? But the code could definitely be simplified.

Screenshot from 2021-12-15 15-29-57

FHachez commented 2 years ago

@kankaristo Thanks for testing it. What did you try?

Ah! It seems that the ratio get reversed because in the past we were using a computation with the padding bottom example:

<iframe src="https://www.youtube.com/embed/mSC6GwizOag" allow="fullscreen" style="aspect-ratio:16/9; object-fit:contain; width: 100%;"></iframe>

<div style="display: block; position: relative; width: 100%; height: 0px; --aspect-ratio:9/16; padding-bottom: calc(var(--aspect-ratio) * 100%);"><iframe src="https://www.youtube.com/embed/mSC6GwizOag" allow="fullscreen" style="position: absolute; top: 0px; left: 0px; height: 100%; width: 100%;"></iframe></div>

image

kankaristo commented 2 years ago

I tried that exact same code (the one you pasted in your latest comment), but it doesn't display the same way for me for some reason (I only added ----- in the Markdown before, after and between the iframes to add some space in between):

Screenshot from 2021-12-15 17-42-45

Could you have some additional CSS on iframes from a theme or CSS snippet? I'm running the default dark theme and even if I turn off all CSS snippets, I'm still getting the above result (I only have border: 0; for iframes in my CSS snippets, but disabling all snippets doesn't affect the above result).

EDIT: I also tried turning on "safe mode" so that all community plugins are disabled, in case they have some extra CSS breaking this, but I still get the same result.

kankaristo commented 2 years ago

Obsidian's DevTools is showing: Screenshot from 2021-12-15 18-27-42

If I hover over the warning, it says "unknown property name". The same CSS works as expected if I try it in Chrome, but the aspect-ratio property seems to be missing in whatever version of Electron Obsidian runs on. I'm on Linux/Ubuntu, and I have the latest version of Obsidian (v0.12.19).

kankaristo commented 2 years ago

Okay, I got it to work now...

According to the release notes for Obsidian v0.12.3 you have to "explicitly" reinstall Obsidian to get the Electron 12 update (Obsidian's internal auto-update can't update it, apparently). And apparently before version 12, Electron didn't support aspect-ratio.

So this change might require a "manual" update of Obsidian for users, but I expect this isn't the only thing that will break eventually, if Electron isn't actually updated alongside Obsidian.

If Obsidian had a package repo for Linux, like most apps, instead of using only Flatpak/Snap/AppImage, this wouldn't be a problem. But using Flatpak/Snap/AppImage is the trendy new thing, so... Hopefully those will mature to have proper package management.

kankaristo commented 2 years ago

So, IMO, go ahead with the change (the code is a lot cleaner), but maybe also add something about this to the readme and/or release notes, since I expect I won't be the only one that will have to reinstall Obsidian to get this to work.

Obsidian is still pre-v1 (although not explicitly "beta"), so I think users should expect things like this, and this probably won't be the only thing eventually breaking, if you have an old version of Electron, so updating it is a smart idea anyway.

FHachez commented 2 years ago

@kankaristo Thanks a lot for the investigation! I agree with you, it's a nice improvement worth the upgrade and we'll require a note in the readme/realease notes.

I plan to work on that this Saturday.

FHachez commented 2 years ago

Done