react-coinbase-commerce icon indicating copy to clipboard operation
react-coinbase-commerce copied to clipboard

`onModalClosed` doesn't work

Open matsko opened this issue 4 years ago • 9 comments

Hello.

When the <CoinbaseCommerceButton> component is instantiated (and the modal opens up), the component fails to render if the onModalClosed property is set.

According to the documentation here, onModalClosed is a valid property, however, the component doesn't look like it has the code in place to recognize it.

Here's what my code looks like:

      <CoinbaseCommerceButton
        className={styles.coinbaseBuyNow}
        onPaymentDetected={processCoinbaseEvent}
        onChargeFailure={processCoinbaseEvent}
        onChargeSuccess={processCoinbaseEvent}
        onModalClose={processModalClose} // this fails
        chargeId={coinbaseChargeId}>
        Buy Now
      </CoinbaseCommerceButton>

I tried both onModalClosed and onModalClose and I got the same error:

Screen Shot 2021-09-09 at 11 22 00 AM

matsko avatar Sep 09 '21 18:09 matsko

Hi @matsko! You might try changing the prop name like so:

onModalClose={processModalClose}

to

onModalClosed={processModalClose} <---- with a 'd'

Let us know if that works for you. 🙏

chrisloeper-coinbase avatar Sep 09 '21 23:09 chrisloeper-coinbase

@chrisloeper-coinbase see my comment above:

I tried both onModalClosed and onModalClose and I got the same error

matsko avatar Sep 09 '21 23:09 matsko

I'm also seeing that error. However, the callback does fire when I user onModalClosed. Is the callback working for you despite the error?

chrisloeper-coinbase avatar Sep 09 '21 23:09 chrisloeper-coinbase

Yes it works. But the error is still showing up.

matsko avatar Sep 20 '21 22:09 matsko

I see, thank you for the heads up! We will prioritize this as we are able. We just wanted to ensure that you were not blocked.

chrisloeper-coinbase avatar Sep 22 '21 17:09 chrisloeper-coinbase

Yes I can confirm that onModalClosed={fn} does indeed work for us. The only problem that remains here is that your still library emits an error when it's used despite it being valid. Could you fix this error? It makes our unit/e2e testing emit the wrong signals.

matsko avatar Oct 21 '21 17:10 matsko

I will talk with the team to see how best to prioritize this work.

chrisloeper-coinbase avatar Oct 21 '21 23:10 chrisloeper-coinbase

@chrisloeper-coinbase I've quickly checked through the code and it seems the only place where the code must be updated to fix this issue is here:

https://github.com/coinbase/react-coinbase-commerce/blob/master/src/index.js#L28

You only need to add 'onModalClosed' to the ignoredProps array

georgii-nansen avatar Dec 21 '21 16:12 georgii-nansen

also this problem exists since 2019 and this issue was simply deleted without any fix/workaround/feedback from the team.

georgii-nansen avatar Dec 21 '21 16:12 georgii-nansen