Added contentWillUnmount prop to call before the Contents unmounts.

[Reflex-Gravity]

This PR contains a new feature which adds a new prop contentWillUnmount that calls before the Content unmounts.
This PR is linked to #178 .

I have also added tests for this feature.

[paztis]

The componentWillUnmount poperty mught be optional. Can you add a null test on this.props.componentWillUnmount before execution ?

[Reflex-Gravity]

I have removed contentWillUnmount from the required props, and set the default value of contentWillUnmount to null. I have also added tests to support this.

[Reflex-Gravity]

@paztis Anything else I need to change? Is there something that is blocking from merging?

[paztis]

No it seems ok. But I'm not owner of the repository and cannot approve it Le mar. 4 mai 2021 à 10:35, Joel D Souza ***@***.***> a écrit :

…

@paztis <https://github.com/paztis> Anything else I need to change? Is there something that is blocking from merging? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#185 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIKMMMFD5CL3PSJC5HJA5LTL6WWLANCNFSM433SLBVA> .

[ryanseddon]

Looking good just some feedback

[ryanseddon]

@Reflex-Gravity some feedback for you before we can merge

[ryanseddon]

I see this is called but this test doesn't check the callCount either?

[Reflex-Gravity]

So, I just need to check the callCount to be 1 ryt?

[ryanseddon]

yep

[Reflex-Gravity]

I have updated this test. But, the test is failing. The contentWillUnmount isn't getting called, maybe because the unmounting of the component in the test isn't happening properly.
I don't exactly get the reason for this. The implementation of the feature seems to be correct and working. But the way I wrote the test is what I guess is wrong. Can you help me here?

[ryanseddon]

Ok so i think what we're doing here is actually the wrong thing, the issue is about having a callback when the <Frame> component unmounts and not when the contents within the iframe unmount.

So I think what we need is a bit simpler instead of passing the contentWillUnmount prop down through to <Content> we just need to call it inside <Frame>'s componentWillUnmount callback and we should probably rename it to frameWillUnmount to differentiate it from the other content lifecycle events.

Then you can just test it like this:

  it('should call contentWillUnmount on unmount', done => {
    div = document.body.appendChild(document.createElement('div'));
    const willUnmount = sinon.spy();
    const frame = ReactDOM.render(
      <Frame frameWillUnmount={willUnmount} />,
      div
    );
    ReactDOM.unmountComponentAtNode(ReactDOM.findDOMNode(frame).parentNode);
    expect(willUnmount.callCount).to.equal(1, 'expected 1 willUnmount');
    done();
  });

[Reflex-Gravity]

Okay. But still contentWillUnmount should be called as Content gets unmounted too.
I tried it in an example, the contentWillUnmount was called when I unmounted the Frame. The test however doesn't trigger the callback.

[ryanseddon]

Yeah ok having though on this more I think what you have is useful in two instances of changing the iframe contents as well as unmounting the entire Frame too.

I honestly have no idea why this isn't firing in test env, if you run npm run karma:dev that will allow you to debug the tests within Chrome dev tools.

[Reflex-Gravity]

Tried with npm run karma:dev same issue. How do I proceed? Skip this test?

[ryanseddon]

@Reflex-Gravity not sure why act() didn't work but this might give you a clue as to what's happening here, srcdoc is async and act doesn't seem to wait long enough for the iframe to actually mount but doing the old setTimeout next tick trick does...

[ryanseddon]

Also these tests should assert something?

[ryanseddon]

Same here nothing is asserted?