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

Responsive: Not rendering correctly with SSR #3110

Closed
teoraunio opened this issue Aug 28, 2018 · 11 comments
Closed

Responsive: Not rendering correctly with SSR #3110

teoraunio opened this issue Aug 28, 2018 · 11 comments
Labels

Comments

@teoraunio
Copy link

teoraunio commented Aug 28, 2018

Bug Report

I am trying to create a top bar which uses two Responsive elements to determine if it should render content for over or under 1000 pixel resolution. The app is using server side rendering.

The problem (code below to understand the props and classes):
When the window size is over 1000 pixels, it will render a Responsive element that has minWidth={1000} as expected, but the first element inside the Responsive is actually from the other Responsive element with maxWidth={1000}

So what happens is:

  1. The server renders the element with maxWidth=1000 as window doesn't yet exist and the width is set to 0 inside the Responsive component.
  2. Client re-renders it according to acquired window width and now chooses to use the Responsive element with minWidth=1000
  3. The <div> element that should have class desktop-content-1 has falsely class mobile-content that is defined inside the other Responsive element.

I am aware that this can be fixed by wrapping both Responsive elements in separate containers, but it means using unnecessary elements and it should at least be specified in the documentation.

Steps

You need an app that uses Server Side Rendering with the following piece of code rendered to reproduce the problem:

<div className="top-bar">
  <Responsive minWidth={1000}>
    <div className="desktop-content-1" />
    <div className="desktop-content-2" />
  </Responsive>
  <Responsive maxWidth={1000}>
    <div className="mobile-content" />
  </Responsive>
</div>

Expected Result

The width of the window is over 1000 pixels.

<div class="top-bar">
  <div>
    <div class="desktop-content-1" />
    <div class="desktop-content-2" />
  </div>
</div>

Actual Result

The width of the window is over 1000 pixels.

<div class="top-bar">
  <div>
    <div class="mobile-content" />
    <div class="desktop-content-2" />
  </div>
</div>

Version

0.77.1

@welcome
Copy link

welcome bot commented Aug 28, 2018

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you've completed all the fields in the issue template so we can best help.

We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

@layershifter
Copy link
Member

layershifter commented Aug 28, 2018

@teoraunio thanks for the detailed report!

  1. Please update to latest version 🐱
  2. The latest version contains the getWidth prop, by default width will be always 0 for SSR. You can override it with this prop.
  3. Use fireOnMount prop, it should solve your issue, because listeners are fired on resize & scroll by default.
const getWidth = () => {
  if (isBrowser) return window.innerWidth
  return 1000
}

<div className="top-bar">
  <Responsive fireOnMount getWidth={getWidth} minWidth={1000}>
    <div className="desktop-content-1" />
    <div className="desktop-content-2" />
  </Responsive>
  <Responsive fireOnMount getWidth={getWidth} maxWidth={1000}>
    <div className="mobile-content" />
  </Responsive>
</div>

If issue will not be solved, just ping me and I will reopen it. Can you please also create an example repo with a preconfigured env, if it will persist?

@teoraunio
Copy link
Author

@layershifter Thanks for your quick reply!

Unfortunately, this did not fix the issue. Now exactly the same is happening if the window width is less than 1000 pixels in the first render.

So when the child div's className should be mobile-content it is desktop-content-1 instead:

<div class="top-bar">
  <div>
    <div class="desktop-content-1">
      {/* correct mobile content inside */}
    </div>
  </div>
</div>

The content inside the div is correct, only the className is incorrect.

I am not able to create an example repo at the moment but I may look into it later on if necessary.

@layershifter layershifter reopened this Aug 28, 2018
@layershifter
Copy link
Member

Yes, please create a repo with a simple example.

@layershifter
Copy link
Member

I've updated a message (fixed getWidth()).

I've checked all should work correct, waiting for a repro repo. Please keep it simple.

@layershifter
Copy link
Member

Will reopen if we will receive a repro case.

@JustinTRoss
Copy link

This didn't work for me either. I'll try to get a repro repo over tomorrow.

@JustinTRoss
Copy link

Nvm. Replacing your getWidth with the following fixed it.

const isBrowser = () => typeof window !== 'undefined'
const getWidth = () => {
  if (isBrowser()) return window.innerWidth
  return Infinity
}

Thanks for the help!

@GGAlanSmithee
Copy link

Thanks for this great feature. I had to use 0 to make both the initial as well as future renders work properly

    const isBrowser = () => typeof window !== 'undefined'
    const getWidth = () => (isBrowser() ? window.innerWidth : 0)

@ukyiwin
Copy link

ukyiwin commented Jan 1, 2019

                const isBrowser = () => typeof window !== 'undefined'
                 export const getWidth = () => (isBrowser() ? window.innerWidth : 0)
                <Responsive fireOnMount getWidth={getWidth} minWidth={1000}>
                    <h1>Desktop</h1>
                    {renderRoutes(routes)}
                </Responsive>
                <Responsive fireOnMount getWidth={getWidth} maxWidth={1000}>
                    <h1>Mobile</h1>
                    {renderRoutes(routes)}
                </Responsive>

in server side getWidth 0 render with Mobile view
and then
client side rerender getWidth's view
How can I replace 0?

@layershifter
Copy link
Member

#3361 (comment)

My comment there contains working solutions.

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

No branches or pull requests

5 participants