search

jasonkuhrt / react-popover

A smart popover component for React
600 stars 240 forks source link

Fix a few migration issues #144

Closed gabehayes closed 7 years ago

gabehayes commented 7 years ago

I broke these up into separate commits, so please feel free to cherry-pick if you aren't satisfied with all.

I just upgraded to React 16 and your latest release and unfortunately still had some issues.

First, 8917a44 fixes an error I was getting when importing. It was unable to resolve build/main, as it does not exist.

499eb4c addresses a bug I ran into that should be reproducible with the following steps:

  1. Load a page with a popover
  2. Open the popover
  3. Close the popover (so exit() is called)
  4. Navigate elsewhere or do something that will cause componentWillUnmount to get called

The issue was that componentWillUnmount and exit both call untrackPopover, but if exit has already been called, an error will be thrown when trying to unbind events.

434bd80 is addressing an SSR issue I have been having. ReactDOM.createPortal requires the second argument to be a DOM element. When isServer evaluates to true, it is being set to null and thus throwing an error when the server attempts to render. The code change will either return false or continue with the createPortal call, allowing the server to render.

jasonkuhrt commented 7 years ago

@gabehayes This is fantastic PR, thank-you! LGTM.

gabehayes commented 7 years ago

Happy to help, thanks for the merge! I upgraded and things are running smoothly with React 16 and SSR 👌