-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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(browser_adapter): fix clearNodes() in IE #3355
fix(browser_adapter): fix clearNodes() in IE #3355
Conversation
6542f57
to
2f1f213
Compare
@@ -130,14 +130,14 @@ export class BrowserDomAdapter extends GenericBrowserDomAdapter { | |||
} | |||
clearNodes(el) { | |||
while (el.firstChild) { | |||
el.firstChild.remove(); | |||
el.removeChild(el.firstChild); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you write a comment that can explain the reason why the refactor is needed or simply a comment with a url to the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gdi2290 this commit has a reference to the original issue, so hopfully it should be enough. And as soon as we plug other browsers on the CI it will be self-verifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right I jumped to conclusion and we have github/git blame
/CI for a reason. If someone were to optimize this they would first check git blame
then the git commit
history before proceeding and if that fails CI will prevail. With all that said, comments are important to show hot path, document unorthodox/failed optimizations, unimportant technical debt via TODO:
, or code documentation
LGTM, this solves issue #3295 in IE11 |
Thnx @Mlaval for the review. I'm going to land it as soon as we get master back to green. |
Please create a presubmit branch... |
2f1f213
to
e95f8fa
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #3295