Reuse icon DOM #1726
Merged
Reuse icon DOM #1726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
May 31, 2013 12:47
The icon should switch from blue to red and back, but does not in IE because the DOMelement has disappeared, as in issue Leaflet#561. Instead, the mouseover event is fired on any mouse motion in the marker. In addition, in FF & Chrome the mouseover and mouseout events are continously fired on any movement, which gives the correct visual results but causes a lot of superflous event handling.
If there already exists a DOM object for this marker, reuse it by setting a new src and style rather than creating a new one.
| img = this._createImg(src); | ||
| this._setIconStyles(img, name); | ||
| } else { | ||
| img = this._createImg(src, oldIcon); |
There was a problem hiding this comment.
This implies that we can only change to a different icon with the same size/anchor/etc., but this is often not the case. So perhaps styles should be updates when reusing the icon too?
There was a problem hiding this comment.
Ah, crap! I did actually have that after the if and changed it to check something, then must have forgotten to change back. I'm 99% sure all we have to do is move this._setIconStyles(img, name); after the else clause and it'll work. I can fix it on Monday.
|
Thanks :) I think it looks quite good! I'll test it a bit after your update and merge if there are no visible regressions. |
This fixes the error found in review.
|
OK, now things should be working as expected! |
|
Awesome, thanks! |
This was referenced Jun 12, 2013
|
Had to rewrite this a bit because of #1768 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes issue #561 and related issues that occur when using setIcon in an eventhandler like dragstart or mouseover. It includes a new debug/tests file that can be used to verify the results.
The fix works by checking if a marker has a pre-existing DOM img element, and only creating a new element if there is none. Otherwise, it will take the existing element and update its src and style information to take on the appearance of the new icon.
No new automated tests have been added, but all the previous ones still pass.