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

[change] deprecate getParentElement in favor of local element. #440

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

diasbruno
Copy link
Collaborator

@diasbruno diasbruno commented Jun 25, 2017

To keep things simple, each Modal can be placed on any level
of the react's tree and it will do a proper clean up when it's
time to unmount the component. Also, it make things simple enough
to deal with overlapped modal.

It will use a local element, created by which will still be visible by
getting node property.

Changes proposed:

  • Use a div provided by the instance of to setup things.
  • Each <Modal /> can perform its own clean up.

Upgrade Path (for changed or removed APIs):

  • Remove getParentElement.

Acceptance Checklist:

  • All commits have been squashed to one.
  • 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.

@diasbruno diasbruno force-pushed the feature/deprecate-get-parent-element branch from 75c4ba2 to efb0954 Compare June 25, 2017 04:59
@diasbruno diasbruno changed the title [feature] deprecate getParentElement in favor of findDOMNode. [feature] deprecate getParentElement in favor of local element. Jun 25, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 84.923% when pulling efb0954 on diasbruno:feature/deprecate-get-parent-element into f5d95e2 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 84.923% when pulling efb0954 on diasbruno:feature/deprecate-get-parent-element into f5d95e2 on reactjs:master.

@diasbruno diasbruno added this to the 3.0 milestone Jun 25, 2017
@diasbruno diasbruno force-pushed the feature/deprecate-get-parent-element branch from efb0954 to 376c4e6 Compare June 27, 2017 16:30
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 85.511% when pulling 376c4e6 on diasbruno:feature/deprecate-get-parent-element into 6f73764 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 85.511% when pulling 376c4e6 on diasbruno:feature/deprecate-get-parent-element into 6f73764 on reactjs:master.

@diasbruno diasbruno force-pushed the feature/deprecate-get-parent-element branch from 376c4e6 to c5b148d Compare June 29, 2017 00:27
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 85.794% when pulling c5b148d on diasbruno:feature/deprecate-get-parent-element into b67ad54 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 85.794% when pulling c5b148d on diasbruno:feature/deprecate-get-parent-element into b67ad54 on reactjs:master.

@diasbruno diasbruno changed the title [feature] deprecate getParentElement in favor of local element. [change] deprecate getParentElement in favor of local element. Jul 6, 2017
@adrianq
Copy link

adrianq commented Aug 17, 2017

Hi! Any plan to get this merged in the short term?

@diasbruno
Copy link
Collaborator Author

Hi @adrianq, I don't know, I believe it still needs some tests and It's also a breaking change, so I'm not sure.

@diasbruno
Copy link
Collaborator Author

If you have some time you can review this PR. :)

@diasbruno diasbruno removed this from the 3.0 milestone Sep 21, 2017
This patch will give us the ability to place the modal
in any place in the react tree. Each modal can handle its own
clean up.

Test case was written by @restrry [b9a3415](mshustov@b9a3415).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
3.0
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants