search

TheSpyder / rescript-webapi

ReScript bindings to the DOM and other Web APIs
http://tinymce.github.io/rescript-webapi/api/Webapi/
Other
149 stars 36 forks source link

Use correct name for setProperty #114

Closed pbiggar closed 1 year ago

pbiggar commented 1 year ago

I'm not certain this is right, as I'm not as familiar with the current incarnation of Rescripts externals.

My read of this code is that setProperty is intended to call setProperty with 2 arguments, and setPropertyValue is supposed to call setProperty with 3 arguments.

There are related functions getProperty and getPropertyValue, but there isn't a setPropertyValue, so my guess is there was a typo here.

TheSpyder commented 1 year ago

I'm not certain either - much of this code was inherited from a previous project, and some of it was written when the standards were still in draft. When in doubt, consult the current standard - there is no setPropertyValue. https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration

setProperty can take 2 or 3 arguments, so I guess the intent is the setProperty binding is the full one that also sets the priority and setPropertyValue just sets the value but not the priority.

Either way this PR is wrong sorry :) maybe just add some doc comments instead?

pbiggar commented 1 year ago

setProperty can take 2 or 3 arguments, so I guess the intent is the setProperty binding is the full one that also sets the priority and setPropertyValue just sets the value but not the priority.

Isn't this what this PR does?

TheSpyder commented 1 year ago

No, because you changed the emitted function call. There is no setPropertyValue in the standard.

This would be clearer if we had a wider range of tests, which is on my list of things to do but that's quite a lot of work 😅

TheSpyder commented 1 year ago

Oh. I'm reading the diff backwards. Sorry, I've had a long week 😞

pbiggar commented 1 year ago

Thanks!

TheSpyder commented 1 year ago

I was planning to publish it immediately but hit a compile error in another merge. Hopefully I can resolve that soon.

TheSpyder commented 1 year ago

Published as 0.7.0