Remove jQuery throttle-debounce dependency#6811
Remove jQuery throttle-debounce dependency#6811hlfan wants to merge 1 commit intoopenstreetmap:masterfrom
Conversation
app/assets/javascripts/id.js
Outdated
|
|
||
| let hashChangedAutomatically = false; | ||
| id.map().on("move.embed", parent.$.throttle(250, function () { | ||
| window.history.replaceState = function () { |
There was a problem hiding this comment.
You do not want to overwrite basic Browser APIs.
https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState
There was a problem hiding this comment.
I've now made it fire a custom event. Is that override acceptable?
hlfan
force-pushed
the
id-no-throttle
branch
from
ddaca19 to
0eb0954
Compare
|
Can we have instead a (throttled) event from iD on its parent like? window.parent?.postMessage('id-hash-update', '*' /* This is no sensitive data, so ok to allow all targets */); |
|
Now we have |
|
Does the second change actually make anything simpler? It certainly increases the amount of code... |
It decreases the coupling between the parent and the iframe. |
hlfan
force-pushed
the
id-no-throttle
branch
from
3b6a74d to
de189a4
Compare
|
My favorite solution would be here in this repo in one file |
|
Yeah that would be nice, or exposing iD's lodash utilities similarly to d3. @tyrasd which option do you think would be more in-scope for the iD repo? |
I've dropped this change for now. |
| window.dispatchEvent(new CustomEvent("replaceHistoryState", { detail: args })); | ||
| return result; | ||
| }; | ||
| })(); |
There was a problem hiding this comment.
This makes me very uneasy. Is this overwrite common? Did @HolgerJeromin have an opinion on this? I saw the comment was resolved.
There was a problem hiding this comment.
I do not like this approach as you know, but it is not unusual in JS world.
4 participants
iD already throttles its hash updates. Hooking into this allows dropping the vendored dependency, leaving only leaflet-related ones.