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

Layout shift with latest version while pre-rendered on the server side #240

Closed
xeladotbe opened this issue Dec 23, 2023 · 17 comments · Fixed by #242
Closed

Layout shift with latest version while pre-rendered on the server side #240

xeladotbe opened this issue Dec 23, 2023 · 17 comments · Fixed by #242

Comments

@xeladotbe
Copy link

In the latest version (1.0.4) you can see a layout shift when the panes are pre-rendered on the server side. With version 0.0.54 (which is also used in the example repository) there is no layout shift.

This example uses the latest version, here you can see the layout shift:
https://codesandbox.io/p/devbox/infallible-bird-forked-y8w6sl?file=%2Fpackage.json%3A21%2C1-22%2C1

This examples uses 0.0.55, there is no layout shift:
https://codesandbox.io/p/devbox/infallible-bird-q9cmdq?file=%2Fpackage.json%3A21%2C1-22%2C1

@senadir
Copy link

senadir commented Dec 23, 2023

Seeing the same issue as well.

@mikdanjey
Copy link

I'm also facing same issue in Nextjs. Please fix this

@bvaughn
Copy link
Owner

bvaughn commented Dec 23, 2023

Please fix this

This is a free library. If you’re offering to pay for a fix, then we can talk about a timeline/urgency. Otherwise, I’ll get to it when I get to it. :)

@senadir
Copy link

senadir commented Dec 23, 2023

It goes without saying, but still needs saying, thank you for the effort @bvaughn

@bvaughn
Copy link
Owner

bvaughn commented Dec 23, 2023

Thanks senadir.

This regression is likely due to a change in the format this component serializes persisted layout data to, which was done in order to support the request in #234. (Even small changes often come with a cost.) I'll think about the best way to fix this. I believe I was mistaken. I think the issue is the way the PanelGroup now defers calculation of style until mount, even if a defaultSize prop has been provided.

bvaughn added a commit that referenced this issue Dec 24, 2023
Panels should fall back to `defaultSize` on initial render.

Fixes #240
@bvaughn
Copy link
Owner

bvaughn commented Dec 24, 2023

This issue has been fixed in 1.0.5.

Happy holidays.


❤️ → ☕ givebrian.coffee

@xeladotbe
Copy link
Author

Hey @bvaughn,

thanks for the fix, unfortunately I still have the layout shifts with the latest version 1.0.5. I have updated the codesandbox link to reproduce the behavior.

Merry christmas!

@bvaughn
Copy link
Owner

bvaughn commented Dec 24, 2023

There was a layout shift for me the first time I loaded that page, but not after that. Resizing and reloading after the first load works fine. I assume maybe you didn't initialize things to the same default on the server and client?

@xeladotbe
Copy link
Author

xeladotbe commented Dec 24, 2023

I have, as you can see in my codesandbox link the layout is stored in a cookie which is read and passed to the client component.

I just looked at your PR, shouldn't it actually be if (size == undefined) { or if (!size) { because size is initalized with const size = layout[panelIndex]; and that doesn't return null, I wrote a test locally for this case and so my static markup is initalized with flex-grow: <value>

/**
 * @jest-environment node
 */

import { renderToString } from "react-dom/server";
import { act } from "react-dom/test-utils";
import { Panel, PanelGroup, PanelResizeHandle } from ".";

describe("PanelGroup", () => {
  it("should render default sizes to html markup", () => {
    act(() => {
      expect(
        renderToString(
          <PanelGroup direction="horizontal" id="group-without-handle">
            <Panel defaultSize={30} />
            <PanelResizeHandle />
            <Panel defaultSize={70} />
          </PanelGroup>
        )
      ).toMatchSnapshot();
    });
  });
});

@bvaughn
Copy link
Owner

bvaughn commented Dec 24, 2023

I just looked at your PR, shouldn't it actually be if (size == undefined) { or if (!size) { because size is initalized with const size = layout[panelIndex]; and that doesn't return null

No, == null is shorthand for === undefined || === null

@xeladotbe
Copy link
Author

xeladotbe commented Dec 24, 2023

here is a video that shows the behavior I see with the latest version

output.mp4

I have a separate, local test Harness using Next.js that confirms the same behavior.

@bvaughn
Copy link
Owner

bvaughn commented Dec 24, 2023

So you're reporting a different behavior than originally. Slight layout shift?

@bvaughn
Copy link
Owner

bvaughn commented Dec 24, 2023

Not sure what to say. Using this Sandbox here is what I'm seeing:

Kapture.2023-12-24.at.17.46.22.mp4

@xeladotbe
Copy link
Author

okay, it must be my chrome, I can't reproduce the behavior in edge and firefox either. I also tried it in chrome in incognito mode, there I have the same problem. (I apologize for the inconvenience!)

I would like to buy you 2 coffees, but I don't have a credit card. are you on another platform where you can pay with paypal?

@bvaughn
Copy link
Owner

bvaughn commented Dec 25, 2023

That’s pretty interesting. I wonder if it has something to do with an extension you have installed or something?

Thats very kind of you to offer. <3 uh…I’m on Venmo and Messenger (Pay) I guess 😅 if you really wanted you could email me and I’ll link you, but dont feel like to have to.

@senadir
Copy link

senadir commented Dec 29, 2023

I can confirm the issue was fixed on my side, FWIW I saw this issue when using the component from shadcn/ui lib.

@EvanBarnesAZ
Copy link

I can confirm the issue was fixed on my side, FWIW I saw this issue when using the component from shadcn/ui lib.

I am having an issue with the component from the shadcn/ui lib.. Did you find a fix by chance?

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 a pull request may close this issue.

5 participants