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

Modal: bug with SSR rendering #2259

Closed
adam-26 opened this issue Oct 26, 2017 · 2 comments
Closed

Modal: bug with SSR rendering #2259

adam-26 opened this issue Oct 26, 2017 · 2 comments

Comments

@adam-26
Copy link
Contributor

adam-26 commented Oct 26, 2017

Steps

See #1531 for steps

Expected Result

Modal should not cause SSR to result in invalid checksum on client

Actual Result

Client checksum is invalid

Version

0.72.0

Testcase

From #1531
https://github.com/kolpav/semantic-ui-ssr-bug

As per the closed issue #1531, rendering a Modal on the server results in an invalid checksum on the client.

The bug is caused in the render() method at this line. The problem is that on the server it does not render the trigger content.

A decent partial solution is to simply change from this (current code @ line 324):

// Short circuit when server side rendering
if (!isBrowser) return null

To this, rendering the trigger component on the server:

// Short circuit when server side rendering
if (!isBrowser) {
  return this.props.trigger;
};

It's not a perfect solution, it will work on the condition that the Modal has a state of closed when rendering on the server. I'm guessing this will be acceptable in most use-cases, as Modal components are generally displayed in response to user interaction and therefore don't need to be rendered on the server.

If this solution would be accepted as a PR, i'll submit one. Please let me know!

Adam.

@layershifter
Copy link
Member

I looked a little, you seem right. Feel free to open PR 👍

The only thing I think is that we should wrapper trigger with isValidElement() and returns null when it's false.

@layershifter layershifter changed the title Modal bug for SSR - with a solution :) Modal: bug with SSR rendering Oct 26, 2017
adam-26 added a commit to adam-26/Semantic-UI-React that referenced this issue Oct 26, 2017
levithomason pushed a commit that referenced this issue Oct 29, 2017
* fix: Modal SSR

Resolve Issue #2259

* Change isBrowser to function

Allow isBrowser to be overridden for SSR tests
@layershifter
Copy link
Member

layershifter commented Oct 30, 2017

Fixed in #2260.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants