Nigecat / obsidian-desmos

Embed graphs directly into your obsidian notes
GNU General Public License v3.0
125 stars 7 forks source link

Allow height and width fields to take css values #64

Open tnichols217 opened 2 years ago

tnichols217 commented 2 years ago

Add dynamic resize support to the width and height options of the graph

Depends on #68 Closes #61

Nigecat commented 2 years ago

I'll be able to take a look over this soonish, but for the time being could you merge the upstream master into this branch? As of a few minutes ago it has some new testing stuff which I'd like to test against this.

Nigecat commented 2 years ago

Also just as a general design thing, I think this would be cleaner if there was a separate enum for the size values which stored the magnitude and unit separately (then just set the unit to px by default) - this would allow the validation to be moved back into the original validation function and would also let the unit validation occur there.

tnichols217 commented 2 years ago

Alright, I probable cant work on this till tonight (8 hours) but ill get the changes applied soon

tnichols217 commented 2 years ago

may i also suggest adding main.js and package-lock.json to the .gitignore

tnichols217 commented 2 years ago

Ive fixed everything listed sorry i messed up the git merging and stuff but should be usable

tnichols217 commented 2 years ago

Oh, another weird thing i noticed was that after the iframe renders and captures the screenshot, it shrinks by a little bit and im not sure why

Nigecat commented 2 years ago

Not sure it how it merged like that but we'll need to see if there's a way to fix it other than just rebasing onto another branch since if its merged it'll probably annihilate the rest of the file history .

Nigecat commented 2 years ago

Oh, another weird thing i noticed was that after the iframe renders and captures the screenshot, it shrinks by a little bit and im not sure why

I can check later but I think that behavior might have already existed.

Nigecat commented 2 years ago

Not sure why but I think that merging also broke the workflows, because I seem to have no way to approve running them now. You might have to switch to a different branch from the latest upstream and copy across the changes...

tnichols217 commented 2 years ago

I dont understand the merge conflicts things? I merged the upstream branch properly into this branch but github still marks it as merge conflict.

tnichols217 commented 2 years ago

Not sure it how it merged like that but we'll need to see if there's a way to fix it other than just rebasing onto another branch since if its merged it'll probably annihilate the rest of the file history .

Afaik, when merging this PR into master, it merges all changes as one commit so it shouldnt effect the file history

tnichols217 commented 2 years ago

Okay i think i finally fixed all the merge conflicts it says that the requested changes need to be addressed, however i marked them all as resolved. I think its all up to you now

tnichols217 commented 2 years ago

Alright I got everything done! hopefully you fix whatever workflows you set up cuz everything is failing, but thanks!

Nigecat commented 2 years ago

Ok I've had a bit more time to have a look at this, you sure this works? I tried running it with the function from #68 and it didn't really seem to work properly (specifically for % since its relative to the parent as opposed to the rest of the relative units which are relative to a constant)...

image

Also Obsidian seems to do its own caching on the codeblocks, so this might still be better as a CSS snippet if thats even possible... otherwise it just isn't rescaling

tnichols217 commented 2 years ago

Oh don't forget percent is parsed as "percent" not "%" due to the parseStringAsEnum function

tnichols217 commented 2 years ago

It doesn't rescale automatically until you reupdate the codeblocks, but even with custom css the svg does not scale when the css is active

Nigecat commented 2 years ago

Oh don't forget percent is parsed as "percent" not "%" due to the parseStringAsEnum function

I used the fixed version I mentioned from #68 to test it

tnichols217 commented 2 years ago

Oh that's unexpected, I'll check it out in a few hours.

tnichols217 commented 2 years ago

Oh this is hillarious image

When you set it as 50%, the iframe size is 50%, but then the svg inside the iframe is also 50%, so it gets set as 50% of 50%, which is 25%. Ill just override the inner value to always be 100% to match the iframe then

tnichols217 commented 2 years ago

However, height percentages do not make sense at all, since if the enclosing div is a percentage and the iframe is a percentage, then it will default to 0px? So how should be handle it?

tnichols217 commented 2 years ago

Mr. @Nigecat do you have any clue who this guy is btw?

Nigecat commented 2 years ago

You sure this does work? Using the latest version on this branch with the latest version of obsidian gives image for the % symbol (and if thats intentional it should probably be removed from the RelativeCSSUnit enum).

And all units seemingly produce the same graph (I tested all of them but didn't have enough screen space to include in screenshot): image not sure if that's intentional or not, if it is then we probably want to consider coalescing the units into a single unit.

This also doesn't appear to be scaling properly, since I'd expect using an n by n graph where n is the same unit would give a graph with a 1:1 aspect ratio. image

Nigecat commented 2 years ago

Plus I don't really see how dynamic CSS units would work, since wouldn't forcing the graph to render at, say, 10% of the container require it to either a) re-render whenever the window is resized or b) skew the aspect ratio? I feel like neither of those would be a particularly good solution, though I'm open to your thoughts on the matter.

tnichols217 commented 2 years ago

Well i first made the pr, it converted the relative unit to an absolute one before rendering, then when the window was resized, the graph would not update until you manually rerender the graph.

tnichols217 commented 2 years ago

You sure this does work? Using the latest version on this branch with the latest version of obsidian gives image for the % symbol (and if thats intentional it should probably be removed from the RelativeCSSUnit enum).

And all units seemingly produce the same graph (I tested all of them but didn't have enough screen space to include in screenshot): image not sure if that's intentional or not, if it is then we probably want to consider coalescing the units into a single unit.

This also doesn't appear to be scaling properly, since I'd expect using an n by n graph where n is the same unit would give a graph with a 1:1 aspect ratio. image

I haven't tested it on the latest release, but ill test it soon