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

feat(Modal): added support for React Portal #796

Merged
merged 13 commits into from
Feb 12, 2018

Conversation

fantua
Copy link
Contributor

@fantua fantua commented Jan 30, 2018

  • Added react-portal package (uses React v16 and its official API for creating portals; has a fallback for React v15).
  • Updated enzyme-adapter-react-16 to latest version for Portals support.
  1. Currently handleBackdropClick is fires for nested modals and close all opened. Need to investigate more on this.
  2. Most test is failing by:
expect(wrapper.childAt(0).children().length).toBe(0);

After using ReactDOM.createPortal it's not more 0. Can I remove this checks?

Need some reviews/comments to current changes.

Refers to #601, #761.

@fantua
Copy link
Contributor Author

fantua commented Jan 30, 2018

cc @TheSharpieOne @supergibbs

@supergibbs
Copy link
Collaborator

I haven't had a chance to mess around with portals yet but would it be easier to just wrap a Modal in another component in your app and tell it to render to a portal? Since reactstrap is still supports react 15 not sure if it should have react 16 stuff built in.

@fantua
Copy link
Contributor Author

fantua commented Jan 30, 2018

I haven't had a chance to mess around with portals yet but would it be easier to just wrap a Modal in another component in your app and tell it to render to a portal?

It's doesn't make sense because currently Modal uses unstable_renderSubtreeIntoContainer. renderSubtreeIntoContainer isn't stable and have some bugs (ex. #601), my changes allow you to:

  • use Portal for 16 (which is stable)
  • fix component logic and structure (currently it's looks a little bit weird)
  • leaves fallback for 15

@fantua fantua changed the title feat(Modal): added support for React Protal feat(Modal): added support for React Portal Jan 30, 2018
@seberik
Copy link
Contributor

seberik commented Feb 7, 2018

Calling destroy in componenDidUpdate breaks the close transition.

@TheSharpieOne
Copy link
Member

Exit animations are not working
Closing a nested model closes all of the modals. (not even clicking the backdrop)
For the tests, you can change them however you need to; just ensure they have good/complete coverage and are actually testing (not just running through the code to get coverage) and pass.

- fixed nested Modals,
- updated react-transition-group to 2.2.1
@fantua
Copy link
Contributor Author

fantua commented Feb 9, 2018

Added new changes:

  • fixed Modal exit animation,
  • fixed nested Modals,
  • updated react-transition-group to 2.2.1

@seberik @TheSharpieOne, guys take a look at this, looks like now all should be good 😃

@seberik
Copy link
Contributor

seberik commented Feb 9, 2018

The exit animation now works but when double-clicking the backdrop or escape-key it will close and re-open the modal.

@fantua
Copy link
Contributor Author

fantua commented Feb 9, 2018

Fixed backdrop double-clicking and tests.

@fantua
Copy link
Contributor Author

fantua commented Feb 12, 2018

Merged with master. Waiting for review.

cc @TheSharpieOne

Copy link
Member

@TheSharpieOne TheSharpieOne left a comment

Choose a reason for hiding this comment

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

LGTM, just a small nit.

@@ -76,7 +76,7 @@ describe('Modal', () => {
);

jest.runTimersToTime(300);
expect(wrapper.childAt(0).children().length).toBe(0);
expect(wrapper.childAt(0).children().length).not.toBe(0);
Copy link
Member

Choose a reason for hiding this comment

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

You can remove all of the checks like this. The previous test was ensuring nothing was inserted into the DOM, but now using react-portal, we will always be inserting react-portal, so these checks are more/less pointless (unless you want to check that react-portal get inserted, but I would also only do that in one test, not all of them.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed those unneeded checks and created new test for this.

@TheSharpieOne TheSharpieOne merged commit 49a7f99 into reactstrap:master Feb 12, 2018
@fantua fantua deleted the modal-portal branch February 13, 2018 01:12
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