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

Move repeated code to removeAllChildren #2339

Closed
wants to merge 1 commit into from
Closed

Move repeated code to removeAllChildren #2339

wants to merge 1 commit into from

Conversation

syranide
Copy link
Contributor

What the title says.

  • Test plan: grunt test

@sophiebits
Copy link
Collaborator

Somewhere else we use .textContent = '' to clear… maybe that's faster?

@syranide
Copy link
Contributor Author

@spicyj I distinctly remember that people have been testing this since forever and using the approach React already uses is the faster. But I'll test. That's funny, it's not even using the fastest one from the test...

@syranide
Copy link
Contributor Author

@spicyj I can't find that anywhere. Perhaps you're thinking of this.updateTextContent('');?

@sophiebits
Copy link
Collaborator

Yeah, I guess that's what I was thinking of.

@syranide
Copy link
Contributor Author

syranide commented Feb 3, 2015

@zpao This seems like a straight-forward refactor to me, agreed? :)

EDIT: Hmm, it went from 3 to 2 locations because of a recent PR, perhaps not as meaningful anymore?

@zpao
Copy link
Member

zpao commented Feb 3, 2015

Ok sure. Let's make lint pass though :) I think this is a special case of assignment so just disable the rule in the file (there were a couple others I did this for in my eslint PR).

@syranide
Copy link
Contributor Author

syranide commented Feb 4, 2015

@zpao Ah, grunt lint is somewhat clogged up with other errors :) anyway, fixed and also updated the license. 👍 ?

@gaearon
Copy link
Collaborator

gaearon commented Mar 26, 2016

We’re trying to get better at reviewing PRs timely but it’s an ongoing process. For now I’m just cleaning up stale PRs that didn’t get merged for one reason or another, and in this case unfortunately it seems like a plain oversight due to it being low priority. Since it’s a tiny change I’m not sure whether you are interested in resubmitting it, but please feel free to. For now, I’m closing as it doesn’t merge cleanly anymore.

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