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

show issue with modal #1712

Conversation

benschac
Copy link
Contributor

@benschac benschac commented Aug 10, 2022

What does this PR do and why?

Investigation into antd and modal issues:

This issue can't be closed until antd hits v5 release.

Antd is using findDomNode, you can see the error in the dev console today.

https://reactjs.org/docs/strict-mode.html#warning-about-deprecated-finddomnode-usage

when we bump up to react 18, findDomNode will break with concurrent mode. You'll see if you run this branch, the modal will close then open and then won't be able to close again. This double render is leveling up the issue in development. This is the exact functionality outlined here:

https://reactjs.org/docs/strict-mode.html#warning-about-deprecated-finddomnode-usage

Strict mode can’t automatically detect side effects for you, but it can help you spot them by making them a little more deterministic. This is done by intentionally double-invoking the following functions:

Which happens with StrictMode on. If we remove or comment out strict mode, the bug where the modal closes and then re-opens goes away.

I think the options are:

  • wait for v5
  • use our own modal
    cc: @jbx-protocol/frontend-core - what do y'all think?

If the answer is wait 1, I can close this stack of PR's and re-open them once antd, hits v5.
If 2. I can start to look into using a modal, and also double check that other components aren't using the depricated findDomNode method.

Additionally, I looked through antd's documentation for a destroy method or prop that would address this issue but it didn't exist.

More info:

The real tell is if you use the react dev tools, you'll see the modal visibility state is false after closing, then the forced rerender opening of the modal.

Screenshots or screen recordings

If applicable, provide screenshots or screen recordings to demonstrate the changes.

Acceptance checklist

@vercel
Copy link

vercel bot commented Aug 10, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
juice-interface-rinkeby ❌ Failed (Inspect) Aug 10, 2022 at 5:25PM (UTC)
1 Ignored Deployment
Name Status Preview Updated
juice-interface ⬜️ Ignored (Inspect) Aug 10, 2022 at 5:25PM (UTC)

@benschac
Copy link
Contributor Author

@benschac
Copy link
Contributor Author

Going to close this stack of PR's so the PR queue doesn't get too packed. I'm just going to wait for antd to hit v5. cc: @jbx-protocol/frontend-core

@benschac benschac closed this Aug 12, 2022
@benschac benschac mentioned this pull request Aug 12, 2022
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

1 participant