| [15:33:09] | <mstenta[m]> | symbioquine paul121 I think I found a small bug, curious your thoughts... |
| [15:33:10] | <mstenta[m]> | https://github.com/farmOS/farmOS/blob/a13a0de10adb3d1588a9cb7727e002b801... |
| [15:33:32] | <mstenta[m]> | Should that be passing element.id instead of element to farmOS.map.create()? |
| [15:34:14] | <mstenta[m]> | I discovered this because I was trying to use farmOS.map.targetIndex(id) to look up an instance by ID, but it didn't work... because instance.target was being set to the whole element instead of just the ID |
| [15:35:00] | <mstenta[m]> | Changing element to element.id fixed it... and everything seems to be working. I can open a PR, I just wanted to check to see if you think that's correct, or if there are any other considerations... |
| [15:42:36] | <paul121[m]> | huh, yeah that makes sense. i didn't realize we had that farmOS.map.targetIndex() helper |
| [15:43:06] | <paul121[m]> | I guess OL must accept an ID or the actual element? |
| [15:49:59] | <mstenta[m]> | Yea I guess so |
| [16:09:32] | <symbioquine[m]> | No, it's better for that function (createMapInstance) to accept the element not the id. That means it can be used in scenarios where the target element doesn't have an id - e.g. a map instance creating by Vue/React/etc. |
| [16:10:43] | <symbioquine[m]> | Otherwise, you'd be forced to generate some kind of id just to use the function where you wouldn't otherwise need one - e.g. multiple instances of maps on the same page or maps within dialogs that are created/dismissed in an ad hoc manner. |
| [16:12:03] | <symbioquine[m]> | Passing the HTMLElement is one of the standard ways of instantiating a OL Map object: https://openlayers.org/en/latest/apidoc/module-ol_Map-Map.html |
| [16:13:00] | <mstenta[m]> | Ah hmm OK so maybe we need to rethink farmOS.map.targetIndex() |
| [16:14:00] | <mstenta[m]> | (Or remove it entirely) |
| [16:14:17] | <symbioquine[m]> | Do we? |
| [16:14:28] | <symbioquine[m]> | It looks like the only thing that needs updating is the doc string for the fn... |
| [16:14:31] | <symbioquine[m]> | https://github.com/farmOS/farmOS-map/blob/4e38001e6567734e4b02938f7fe66f... |
| [16:14:37] | <symbioquine[m]> | Should also work if passed an element right? |
| [16:15:12] | <mstenta[m]> | Yea maybe |
| [16:15:26] | <symbioquine[m]> | I'd need to test, but I think comparing element instances with === should work. |
| [16:17:12] | <symbioquine[m]> | https://github.com/symbioquine/farmOS_land_drawing_tool/blob/a5067047ad2... |
| [16:18:12] | <mstenta[m]> | Just to clarify, I wasn't suggesting we change what's passed to Drupal.behaviors.farm_map.createMapInstance() - that would still be an element |
| [16:19:06] | <mstenta[m]> | Here's the PR I opened (just before I saw your chats): https://github.com/farmOS/farmOS/pull/672 |
| [16:19:10] | <symbioquine[m]> | ^ Assumes the passed element has an id though. |
| [16:19:44] | <mstenta[m]> | That's true - that was the original assumption in farmOS-map, but no longer necessary I guess |
| [16:19:55] | <mstenta[m]> | (Back when targetIndex() was added) |
| [16:20:12] | <mstenta[m]> | (Closed that PR) |
| [16:20:44] | <symbioquine[m]> | mstenta[m]: Agreed. Seems like there's actually a few places in the farmOS-map docs that should be updated to reflect that we just pass the input through to OL - thus can accept the same argument types. |
| [16:23:54] | <mstenta[m]> | > I'd need to test, but I think comparing element instances with `===` should work. |
| [16:23:54] | <mstenta[m]> | Confirmed that this works 👍️ |
| [16:27:08] | <toino[m]> | Wtf |
| [16:27:20] | <symbioquine[m]> | > <@mstenta:matrix.org> > I'd need to test, but I think comparing element instances with `===` should work.... (full message at <https://libera.ems.host/_matrix/media/v3/download/libera.chat/975e55db3e...) |
| [16:28:02] | <symbioquine[m]> | ^ Tested the scenario that === doesn't somehow depend on having an id set for HTMLElement. |
| [19:28:17] | * farmBOT has joined #farmos |