Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Implements destroy() method #162

Merged
merged 4 commits into from
Dec 11, 2015
Merged

Implements destroy() method #162

merged 4 commits into from
Dec 11, 2015

Conversation

Indigo744
Copy link
Collaborator

Finalize #117 and fix last leak in #118!

@Indigo744
Copy link
Collaborator Author

Wait for merging. There are problems with the UT and the way I destroy everything.
It seems that there is an issue with how we handle the resize event $(window).on("resize").

@Indigo744
Copy link
Collaborator Author

Ok now it is working.
To explain a little bit, I had some problem because of the resize event attached on the window. I initially forgot to detach it in the destroy function, and thus it was kept between instance (and tests!).
In order to detach if using off, I needed to pass also the eventHandler callback to be sure I detach the correct one. Hence why the refactorization of the code regarding the map resizing.

neveldo
neveldo reviewed Dec 11, 2015
View reviewed changes
js/jquery.mapael.js
// Empty the container (this will also detach all event listeners)
self.$container.empty();
// Detach the global resize event handler
if (self.onResizeEvent) $(window).off("resize." + pluginName, self.onResizeEvent);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just call $(window).off("resize." + pluginName); ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have two maps you will detach all events in that case :-)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, I didn't thought about that case :)

@neveldo
Copy link
Owner

neveldo commented Dec 11, 2015

Great work, thank you ! I merge your PR :)

neveldo added a commit that referenced this pull request Dec 11, 2015
@neveldo
Merge pull request #162 from Indigo744/Indigo744-patch-newarch-destroy
a703b35 
Implements destroy() method
@neveldo neveldo merged commit a703b35 into neveldo:master Dec 11, 2015
@Indigo744 Indigo744 deleted the Indigo744-patch-newarch-destroy branch December 11, 2015 12:53
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Indigo744 @neveldo