Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix(infiniteGrid): fix image check module in IE9 #310

Merged
merged 1 commit into from
Jul 26, 2016
Merged

fix(infiniteGrid): fix image check module in IE9 #310

merged 1 commit into from
Jul 26, 2016

Conversation

sculove
Copy link
Contributor

@sculove sculove commented Jul 18, 2016

Issue

#309

Details

IE9 couldn't unaware load event of images

Preferred reviewers

@kishu

@mixed
Copy link
Member

mixed commented Jul 21, 2016

LGTM

@@ -824,8 +824,10 @@ eg.module("infiniteGrid", ["jQuery", eg, window, document, "Outlayer"], function
$(e.target).off("load error");
checkCount <= 0 && callback && self.core && callback.apply(self);
};
var $el;
Copy link
Member

Choose a reason for hiding this comment

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

Is necessary to declare outside of function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not necessary. I want to declare a variable before running each method.

Copy link
Member

Choose a reason for hiding this comment

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

I think is creating unused closure scope.

Copy link
Contributor

@jongmoon jongmoon Jul 25, 2016

Choose a reason for hiding this comment

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

@netil I think he(@sculove) worries about performance. But It does not make matter anymore.
http://jindo.dev.naver.com/jsMatch/index.html?d=256&openResult=1

So I'm OK to remove var $el. but current code looks fine to me.

Anyway I don't think there will be closure issue. I think $el will be remove from memory after this function is ended.

@taihoon
Copy link

taihoon commented Jul 26, 2016

LGTM

@sculove sculove merged commit ed3e3b0 into master Jul 26, 2016
@sculove sculove deleted the ie9#309 branch August 4, 2016 08:24
netil pushed a commit that referenced this pull request Dec 15, 2016
Related on below commit, which it wasn't applied for Android 2 environment.
05bbcf4

Fix #305 
Fix #306
Close #310
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants