everweij / react-laag

Hooks to build things like tooltips, dropdown menu's and popovers in React
https://www.react-laag.com
MIT License
906 stars 44 forks source link

Not working with AnimatePresence in 2.0 #53

Closed rijk closed 3 years ago

rijk commented 3 years ago

Based on the example from the release notes:

function New() {
  const [ isOpen, setOpen ] = React.useState( false )

  const { layerProps, triggerProps, renderLayer } = useLayer({ isOpen })

  return (
    <AnimatePresence>
      <button onClick={() => setOpen( !isOpen )} {...triggerProps}>Trigger</button>
      {isOpen && renderLayer( <motion.div {...layerProps}>Layer</motion.div> )}
    </AnimatePresence>
  )
}

This triggers the following error:

Uncaught Error: react-laag: Could not find a valid reference for the layer element. There might be 2 causes:
   - Make sure that the 'ref' is set correctly on the layer element when isOpen: true. Also make sure your component forwards the ref with "forwardRef()".
   - Make sure that you are actually rendering the layer when the isOpen prop is set to true

It does work when I replace the <AnimatePresence> with a Fragment (<>).

everweij commented 3 years ago

Thanks @rijk , Can you try the example from https://github.com/everweij/react-laag#animations ? I'm confident that by layout out the elements that way it should work. I have no clear idea why your example isn't working though. The error is fired when isOpen is set to true, but the layer isn't mounted yet afterwards. Apparently, the location of AnimatePresence presence matters in when what gets rendered. My advice for now would be to put AnimatePresence inside renderLayer:

return (
  <>
     <button onClick={() => setOpen( !isOpen )} {...triggerProps}>Trigger</button>
    {renderLayer(
      <AnimatePresence>
        {isOpen && <motion.div {...layerProps}>Layer</motion.div>}
      </AnimatePresence>
    )}
  </>
)
rijk commented 3 years ago

Wouldn't that make rendering a lot heavier. Say you have 100 menu triggers on the page, they will all rerender and reposition on e.g. window resize?

everweij commented 3 years ago

That is a valid concern, but I think this will not be so bad. v2 has a lazy strategy regarding monitoring positioning, meaning that only layers that are open are affected. Also I did some perf-measuring in a production setting (node.env === 'production' / without animations though). On a macbook pro (2017) calculating a single position for 1 layer takes ~ less than 0.1ms. In other words, it can take quite a lot. Haven't tested it with lots of animations though. Will put this on my todo-list to pick up somewhere this week :)

rijk commented 3 years ago

Cool. I will try this alternate implementation and let you know how the performance is. I thought it was different from the 1.8 implementation, but looking closer I noticed it is the actually the same as what I had in 1.8:

<ToggleLayer
  renderLayer={({
    isOpen,
    layerProps,
    layerSide,
    arrowStyle,
  }: RenderLayerProps ) => (
    <AnimatePresence>
      {isOpen && ( ... )}
    </AnimatePresence>
  )}
>
  ...
</ToggleLayer>

So it shouldn't introduce a new performance issue.

rijk commented 3 years ago

Ok, so my bad, I changed the implementation and it's working fine. Thanks!