Refactor to use React component life-cycle methods as they were intended to be used

[begincalendar]

Are there any compelling reasons as to why the Modal component does not utilise the React component life-cycle methods (e.g. componentWillReceiveProps), as the React design intends them to be used?

The implementation obviously works, but because it is unorthodox (in at least my opinion), issues such as these have arisen (and wouldn't have existed if this approach had not been taken):
#71 #291

If there are no compelling reasons, should we look at refactoring the implementations of those life-cycle methods?

[claydiffrient]

The modal was created when react was still very young. When Ryan created it, he was still getting started doing a lot of react stuff and it was built to meet a need we had at Instructure.

Based on what the component lifecycle methods the modal utilizes do, I don't see much that seems unorthodox. I mean, sure there's some rendering and stuff that goes on, but at least when the modal was created it was experimental and has turned out to be successful. I don't think these implementations have caused the root of the problem with testing.

[begincalendar]

The modal was created when react was still very young. When Ryan created it, he was still getting started doing a lot of react stuff and it was built to meet a need we had at Instructure.

That's fair enough.

Based on what the component lifecycle methods the modal utilizes do, I don't see much that seems unorthodox.

The fact that the Modal.render() method doesn't return anything useful and that the actual rendering is done in other life-cycle methods (e.g. componentWillReceiveProps, which is supposed to return a boolean), that doesn't seem unorthodox (or in need of refactoring) to you?

I mean, sure there's some rendering and stuff that goes on, but at least when the modal was created it was experimental and has turned out to be successful.

As I said, the implementation works great, but it falls down with regard to testing in my opinion and that is a deal breaker for me.

I don't think these implementations have caused the root of the problem with testing.

What has then, in your opinion?

[claydiffrient]

The fact that the Modal.render() method doesn't return anything useful and that the actual rendering is done in other life-cycle methods (e.g. componentWillReceiveProps, which is supposed to return a boolean), that doesn't seem unorthodox (or in need of refactoring) to you?

Since the modal uses the "portal" approach, this is fully acceptable, even expected. See this and this.

What has then, in your opinion?

I believe the difficulty in testing is because of the setup required to properly test it. We have the .portal property that allows you to access the contents. It's documented in the README though. While it may be a pain, it's totally possible to be done. See this example from canvas-lms (the original consumer of react-modal).

Don't get me wrong, I'm not saying the modal is perfect by any means or that we aren't open to change. In fact, #112 is a proposed solution to avoid many of these issues. It will likely be part of our v2.

[begincalendar]

Since the modal uses the "portal" approach, this is fully acceptable, even expected. See this and this.

Personally, I'd say that's up for debate.

I believe the difficulty in testing is because of the setup required to properly test it. We have the .portal property that allows you to access the contents. It's documented in the README though. While it may be a pain, it's totally possible to be done. See this example from canvas-lms (the original consumer of react-modal).

I'm aware that it's possible to do testing with react-modal, but yes it is unnecessarily painful, in my opinion.

In fact, #112 is a proposed solution to avoid many of these issues. It will likely be part of our v2.

I don't see why it's even necessary to render using any kind of portal at all.

I think I'm just going to implement my own base Modal component, with only the limited set of features I require.

If you'd like to close this issue, be my guest, otherwise I'm happy for it to stay open for further discussion.

[diasbruno]

@begincalendar can you get an experimental branch implementing this?
@claydiffrient I think If we can benefit from this, we should go for it.

Checklist:

• react < 15.x.y?
• can it help to improve testing?
• improves the render?

[claydiffrient]

@diasbruno I'm all for doing anything that helps the library. I would add to your checklist that it maintains/improves the accessibility story for the modal. That is the strongest point of the portal approach in my opinion at least.

To note, almost all other major React modal libraries use portals:

• Pivotal UI
• react-overlays
• Instructure-UI
• Semantic UI
• And I'm sure many more...

[diasbruno]

...i mean, if we get this implementation working and it proves to be better than the current implementation, i think, there is no reason to miss this improve.

[begincalendar]

I can see now that the portal approach is justified for the feature-set that this library provides, thank you @claydiffrient for the enlightenment.
For anyone else seeking some more info on why portals can be helpful, try reading the answers in this question.
The main takeaway that I got from it was that portals make life easier when it comes to handling more than one open modal at once. I hadn't considered this because my particular use case does not require more than one modal open at once.

Having said that, an inevitable consequence of the current implementation that it makes testing arduous (relative to components that don't need to children outside of their parent).

An ideal would be to have the best of both worlds (ability to render the modal outside of the parent, but implemented with greater alignment to the React "intended way", to make testing easier).

@claydiffrient #112 does appear to be a step towards that ideal (in that it does at least give you the option to do away with notion of portals, which suits my particular use case and it decouples from document.body), but to me it seems like it would unravel at scale (e.g. if <GatewayProvider> components end up overlapping each other).

For me personally, I'm going to stick with my own bespoke implementation of a base modal component, only because I require a fraction of the features that this library provides (hence I don't want to pay for features I don't need, with more verbose tests) and I was already wrapping this library's <Modal> component in my own base modal component anyway.

[claydiffrient]

@begincalendar I'm glad I could assist. I appreciate you bringing this up for us as well. I do think you are right about the ReactGateway approach. I'm hoping that we might be able to contain it within the modal and keep things to the same scale as using react-modal like it is now.

In any case, I'm going to go ahead and close the issue because it seems we figured out what's going on here :)