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

UI: fix deprecated ReactDOM.findDOMNode calls #20368

Merged
merged 4 commits into from
Dec 23, 2022

Conversation

thebuilder
Copy link
Contributor

Issue: #20140

What I did

  • Rewrote the existing ReactResizeDector to use useResizeDetector. This ensures findDOMNode() is not called, since the hook returns a ref that's passed to the monitored element.
  • Added a nodeRef to <Draggable> in UI Container, to prevent it from calling findDOMNode()

The code/addons/jest/src/components/Panel.tsx had to be refactored, since it creates a Content element that, then creates an internal map to render out all the Test panels. This prevented nested hooks/refs from being used, so moved the code into a TestPanel component.
Unsure if we should just get rid of Content entirely, since it seems to only exist to apply flex on a wrapping <div>.

@ndelangen ndelangen changed the title ui: fix deprecated ReactDOM.findDOMNode calls UI: fix deprecated ReactDOM.findDOMNode calls Dec 21, 2022
@ndelangen ndelangen self-assigned this Dec 21, 2022
@ndelangen
Copy link
Member

@thebuilder have you testing this locally?

I'm looking at this PR's deployed storybook here:
https://635781f3500dd2c49e189caf-zffgrziwgh.chromatic.com/?path=/story/storybook-blocks-blocks-anchor--default

And I see the warnings in the console... 😢

@thebuilder
Copy link
Contributor Author

Seems like another instance of <Draggable> is in the codebase. I'll do another search for it.

@thebuilder
Copy link
Contributor Author

@thebuilder have you testing this locally?

I'm looking at this PR's deployed storybook here: 635781f3500dd2c49e189caf-zffgrziwgh.chromatic.com/?path=/story/storybook-blocks-blocks-anchor--default

And I see the warnings in the console... 😢

Had a look - There was actually two instances of <Draggable> in the container. One for nav and one for the addon panel. Missed the one for the addon panel. Pushed an update that should handle it.

@thebuilder
Copy link
Contributor Author

thebuilder commented Dec 21, 2022

Looks good! No errors on Chromatic now. https://635781f3500dd2c49e189caf-pxjngorqfm.chromatic.com/?path=/story/storybook-blocks-blocks-anchor--default

image

Is there an issue for the warning about /runtime.mjs not being used after after preload?

/sb-preview/runtime.mjs was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it has an appropriate as value and it is preloaded intentionally.

@ndelangen
Copy link
Member

Is there an issue for the warning about /runtime.mjs not being used after after preload?

That's fine. it's kinda expected..

Thanks for this awesome work!

@ndelangen ndelangen self-requested a review December 22, 2022 15:37
@thebuilder
Copy link
Contributor Author

thebuilder commented Dec 22, 2022

Is the failing checks expected? Missing playwright executable for some reason 🤔

browserType.launch: Executable doesn't exist at /ms-playwright/chromium-1041/chrome-linux/chrome
╔══════════════════════════════════════════════════════════════════════╗
║ Looks like Playwright Test or Playwright was just updated to 1.29.1. ║
║ Please update docker image as well.                                  ║
║ -  current: mcr.microsoft.com/playwright:v1.29.0-focal               ║
║ - required: mcr.microsoft.com/playwright:v1.29.1-focal               ║
║                                                                      ║
║ <3 Playwright Team                                                   ║
╚══════════════════════════════════════════════════════════════════════╝

@ndelangen
Copy link
Member

Merging in the next will likely help with the PlayWright version issue, @thebuilder

Be aware we're currently having CI issues because babel/babel#15301 has yet to be released.

@jonniebigodes
Copy link
Contributor

@thebuilder, this is looking amazing. Thanks so much for the contribution. If you're ok with it, can you message me on our Discord Server (same username) as I wanted to go over with you a small item related to this pull request?

Looking forward to hearing from you.

Have a fantastic day

Stay safe

@ndelangen ndelangen merged commit d2f66cb into storybookjs:next Dec 23, 2022
@thebuilder thebuilder deleted the ui/fix-find-dom-node branch December 23, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants