Added position related APIs. #705
Conversation
core/source/Dom.mint
Outdated
| } | ||
| */ | ||
| fun offsetLeft (element : Dom.Element) : Number { | ||
| `#{element}.offsetLeft || -1` |
There was a problem hiding this comment.
Returning -1 is here is IMO totally unexpected and may lead to hard-to-debug bugs.
Also, AFAIU it's not coherent with this method documentation.
ditto below
There was a problem hiding this comment.
We have other functions that return values similarly https://github.com/mint-lang/mint/blob/master/core/source/Dom.mint#L371 https://github.com/mint-lang/mint/blob/dom-position-things/runtime/src/normalize_event.js#L93 We could return Maybe(Number) (and for all similar functions) but it would greatly increase the friction when using this.
I'll update the documentation comment to reflect this.
There was a problem hiding this comment.
If the element is not in the DOM, it returns 0:
document.createElement("div").offsetLeft
So maybe returning 0 if it's not applicable is better?
There was a problem hiding this comment.
0 sounds better to me, although it still seems more like a hack tbh...
There was a problem hiding this comment.
I guess it's good for now, yet it needs to be changed for other props as well.
gdotdesign
force-pushed
the
dom-position-things
branch
from
f413d2f to
29e0c8d
Compare
- `Dom.offsetLeft`, `Dom.offsetTop` - `Html.Event.layerX`, `Html.Event.layerY` - `Html.Event.offsetX`, `Html.Event.offsetY` Fixes #498
gdotdesign
force-pushed
the
dom-position-things
branch
from
29e0c8d to
c5bfd40
Compare
| case "layerX": | ||
| return -1; | ||
| case "layerY": | ||
| return -1; | ||
| case "offsetX": | ||
| return -1; | ||
| case "offsetY": | ||
| return -1; |
There was a problem hiding this comment.
Aren't these need to be changed (to 0) as well?
Added:
Dom.offsetLeft,Dom.offsetTopmethodsHtml.Event.layerX,Html.Event.layerYfieldsHtml.Event.offsetX,Html.Event.offsetYfieldsFixes #498