Added className prop support

[benkeen]

I'm actually new to hooks, but I believe this follows the right principles.

Fixes #55.

[benkeen]

Not the prettiest test, but it helped to confirm the classes were being dynamically added/removed from the tippy element.

[atomiks]

Can you add a test that checks the add/remove when updating the state? e.g. starts with className A, after updating, ends with className B, but doesn't still contain className A. the useEffect looks correct to me anyway but still :-)

[benkeen]

I'm afraid I may need a hand here. Two things, actually:

• from what I've read, useLayoutEffect does seem to be the right method to choose for this, but it throws a warning when running the test: "Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded..." (see full message: https://github.com/facebook/react/pull/14596/files). I'm not sure how to suppress that. Or possibly it's just not working properly when run in the test.
• secondly, I couldn't get the test to work as expected (Possibly it's actually a bug in the code?). Rerendering the component shows the old class didn't get removed - but testing manually in the browser seems okay.

[atomiks]

To be honest, I don't really know either. There seems to be missing documentation for this or maybe I can't find it??? 😢

[KubaJastrz]

useLayoutEffect runs synchronously after each render, so it's good for things like DOM manipulation outside React (like plain DOM api, or jQuery). I'm gonna take a look at this PR today

[KubaJastrz]

I changed it back to useEffect and it seems like rerender() doesn't invoke useEffect cleanup function. However, React in browser environment does, so that's why it is working when testing manually.

I have no idea how to fix it either.

[atomiks]

I will go ahead and merge this since it's working fine in the browser and logic looks correct 🤷‍♂️ If we can figure out how to test it, make a PR

[atomiks]

I removed the server test which was causing the warning -- the first test tests the same behavior now anyway (unlike in v1).

[atomiks]

Okay, it's weird, this seems to work fine:

function Test({ className }) {
  useLayoutEffect(() => {
    document.body.classList.add(className)
    return () => {
      document.body.classList.remove(className)
    }
  }, [className])

  return null
}

test('it', () => {
  const { rerender } = render(<Test className="one" />)
  expect(document.body.className).toBe('one')
  rerender(<Test className="two" />)
  expect(document.body.className).toBe('two')
  // Works fine, shows the cleanup function gets invoked
})

[atomiks]

Ok just tested in the browser and it turns out there is indeed a bug. lol!

If you start off with a non-empty className, e.g. hello, then update it later with something else, it is leaving the stale one behind.

The reason is you're adding it in the mounting effect but that one doesn't get cleaned up, unlike the update one

[benkeen]

haha, thanks @atomiks - good catch. That scenario didn't even cross my mind!