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 for bug 247937 #242

Merged
merged 10 commits into from
Jan 24, 2018
Merged

Fix for bug 247937 #242

merged 10 commits into from
Jan 24, 2018

Conversation

tbelcheva
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 96.977% when pulling 48239dc on tbelcheva:fix-247937 into a8ddea3 on IgniteUI:master.

@@ -230,6 +230,9 @@ export class IgControlBase<Model> implements DoCheck {
if (o1 === o2) { return true; }
if (o1 === null || o2 === null) { return false; }
if (o1 !== o1 && o2 !== o2) { return true; }// NaN === NaN
if (o1.nodeName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@tbelcheva The fix is ok, however there might be a case where the customer object has "nodeName" property and in this case the equalsDiff will not work. Please add some more conditions in order to make sure that this is indeed a DOM node. Here is some suggestion: https://stackoverflow.com/questions/384286/javascript-isdom-how-do-you-check-if-a-javascript-object-is-a-dom-object

Copy link
Contributor

@MayaKirova MayaKirova left a comment

Choose a reason for hiding this comment

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

Also please add a test to cover the newly added code for this scenario.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 96.977% when pulling 7f0626e on tbelcheva:fix-247937 into e5aea46 on IgniteUI:master.

@coveralls
Copy link

coveralls commented Jan 18, 2018

Coverage Status

Coverage decreased (-0.01%) to 97.045% when pulling c28746a on tbelcheva:fix-247937 into a273fa2 on IgniteUI:master.

}
});
TestBed.compileComponents().then(() => {
debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the debugger.

@@ -230,6 +230,13 @@ export class IgControlBase<Model> implements DoCheck {
if (o1 === o2) { return true; }
if (o1 === null || o2 === null) { return false; }
if (o1 !== o1 && o2 !== o2) { return true; }// NaN === NaN
var isDOMnode = typeof Node === "object" ? o1 instanceof Node :
Copy link
Contributor

Choose a reason for hiding this comment

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

@tbelcheva Please put all the code in a separate function(s) and also add tests for them.

@mpavlinov mpavlinov merged commit 81ee7e0 into IgniteUI:master Jan 24, 2018
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.

None yet

5 participants