-
Notifications
You must be signed in to change notification settings - Fork 91
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
Fix flash of tooltip content on DOM using SSR + JSX content #13
Conversation
Sure, I'll review that later on today (max tomorrow), because I'm busy currently. |
At first glance, it looks like it's fine. Do we have any test case scenario to reproduce and check if it is working correctly? |
I used Next.js 7 to test the SSR which is where I found the problem. We could add that to devDeps and I should also commit the index.js test environment |
a557fbb
to
04f6160
Compare
I've had to upgrade to Babel 7 to get the environment working... To reproduce, create a import Tippy from '../src/Tippy'
export default () => (
<Tippy content={<div>Tooltip</div>}>
<button>Example</button>
</Tippy>
) Then The content should not be rendered on the DOM (view source). Edit the component source to the old version to see the problem. |
I've added this test case which passes for this version and fails for the previous one: test('tooltip content is not rendered to the DOM', () => {
expect(
ReactDOMServer.renderToString(
<Tippy content={<div>Tooltip</div>}>
<button />
</Tippy>
).includes('<div>Tooltip</div>')
).toBe(false)
}) Removed |
It appears everything is good to go 💯 |
Fixes #12. Apparently we need to use
setState()
because of a warning created when using a portal with SSR. All tests are passing, behavior appears to be exactly the sameReviews?
@KubaJastrz
@elya158