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

[fixed] Call parent.removeChild only if parent exists #778

Merged
merged 3 commits into from
Nov 15, 2019

Conversation

husseyexplores
Copy link
Contributor

Fixes #769

Should we also check before calling removeChild here (In componentDidUpdate()?

const { prevParent, nextParent } = snapshot;
if (nextParent !== prevParent) {
  prevParent.removeChild(this.node);
  nextParent.appendChild(this.node);
}

I'm fairly new to contributing to open-sourced projects. Please let me know if I made any mistakes. Thank you!


Acceptance Checklist:

  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@coveralls
Copy link

coveralls commented Oct 11, 2019

Coverage Status

Coverage decreased (-0.2%) to 86.517% when pulling 35aa2ad on husseyexplores:fix-for-#769 into 4ac3ff4 on reactjs:master.

@diasbruno
Copy link
Collaborator

diasbruno commented Oct 11, 2019

Hi @husseyexplores, Thanks for the PR. As mentioned in the issue, you can add a warning telling that the element has gone.

@husseyexplores
Copy link
Contributor Author

Thanks for the follow-up!
By warning, you mean console.warn, right?

@diasbruno
Copy link
Collaborator

Yep. 👍

@husseyexplores
Copy link
Contributor Author

Added the warning message. Let me know if it needs to be rephrased.

@diasbruno
Copy link
Collaborator

Here is an example of a message we are already using https://github.com/reactjs/react-modal/blob/master/src/components/ModalPortal.js#L93.

This should be the last thing.

@diasbruno
Copy link
Collaborator

Thanks, @husseyexplores. I'll review it.

@diasbruno diasbruno self-requested a review October 27, 2019 00:46
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.

Failed removeChild function for parent when parentSelector is changed
3 participants