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

Fix flash of tooltip content on DOM using SSR + JSX content #13

Merged
merged 9 commits into from Oct 17, 2018

Conversation

atomiks
Copy link
Owner

@atomiks atomiks commented Oct 17, 2018

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 same

Reviews?

@KubaJastrz
@elya158

@KubaJastrz
Copy link
Contributor

Sure, I'll review that later on today (max tomorrow), because I'm busy currently.

@KubaJastrz
Copy link
Contributor

KubaJastrz commented Oct 17, 2018

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?

@atomiks
Copy link
Owner Author

atomiks commented Oct 17, 2018

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

@atomiks
Copy link
Owner Author

atomiks commented Oct 17, 2018

I've had to upgrade to Babel 7 to get the environment working...

To reproduce, create a pages directory with index.js containing:

import Tippy from '../src/Tippy'

export default () => (
  <Tippy content={<div>Tooltip</div>}>
    <button>Example</button>
  </Tippy>
)

Then npm run ssr and open localhost:3000

The content should not be rendered on the DOM (view source). Edit the component source to the old version to see the problem.

@atomiks
Copy link
Owner Author

atomiks commented Oct 17, 2018

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 next as it's not necessary after this.

@atomiks
Copy link
Owner Author

atomiks commented Oct 17, 2018

It appears everything is good to go 💯

@atomiks atomiks merged commit d5f9f52 into master Oct 17, 2018
@atomiks atomiks deleted the ssr-flash-fix branch October 17, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants