Skip to content

Conversation

@Indigo744
Copy link
Collaborator

Hello,

PR #343 introduces a regression: the window onResize event was no longer properly detached.

This PR fixes that (and also fixes some tests wrongly using globals).

@Indigo744 Indigo744 requested a review from neveldo November 3, 2017 09:56
@Indigo744 Indigo744 mentioned this pull request Nov 3, 2017
@neveldo
Copy link
Owner

neveldo commented Nov 7, 2017

Hello @Indigo744,

As your PR contains a lot of changes, I will take the proper time to review it. I give you a feedback asap !

@Indigo744
Copy link
Collaborator Author

There are actually not that much 😄
The most important thing is for the onResize event.
For the tests, there are only a lot of "find & replace" because now I could use the this.$map instead of the selector.

Copy link
Owner

@neveldo neveldo left a comment

Choose a reason for hiding this comment

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

Hello @Indigo744 ,

You are right, I was a bit impressed by the length of the modification at first sight :) . Good fix, I forgot that the onResizeEvent() function was used to detach the event into the destroy() function ! Also, great update & clean of the unit tests !

(Don't you think that the beforeEach/afterEach functions could be factorized (maybe in const.js ?))

@Indigo744
Copy link
Collaborator Author

Right! I factorized it and resolve some remaining comma'first styles.

@neveldo
Copy link
Owner

neveldo commented Nov 9, 2017

Nice, it's ok for me !

@Indigo744 Indigo744 merged commit 156d4e2 into neveldo:master Nov 9, 2017
@Indigo744 Indigo744 deleted the fix_testr branch November 9, 2017 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants