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

Document about potential issues with remounting #172

Closed
yangshun opened this issue Jul 9, 2023 · 8 comments
Closed

Document about potential issues with remounting #172

yangshun opened this issue Jul 9, 2023 · 8 comments

Comments

@yangshun
Copy link

yangshun commented Jul 9, 2023

Background

Hi Brian 👋🏻 , thanks for this awesome library! I'm building a window tiling manager (similar to VS Code) on top of this library and it renders a dynamic grid of panels which can be nested indefinitely. Here's the demo if you're interested (there are still some bugs but the general idea is there).

Issue

I ran into some issue regarding <Panel>s not showing the correct width after remounting. Here's a simplified repro of the issue: https://stackblitz.com/edit/stackblitz-starters-eac7cb?file=src%2FApp.tsx:

  1. Click on the "Remount" button and you can see the the right panel is reduced to a width of 0.
  2. Click on the red divider and the panels appear correctly.

After some digging, I think the problem is with the right panel remounting and having a new internal ID, hence there's no existing size available in the PanelGroupContext and it renders with 0 width. I kinda solved the issue by passing the right <Panel> a stable id. Please lemme know if there's a better way.

Suggestion

I'm not sure if this is considered a bug, but I think it'd be useful to document somewhere that when doing dynamic/conditional rendering of <Panel>s, there's a potential issue with remounting and that stable ids can be passed in to prevent the issue.

@albertodeago
Copy link

@yangshun I'm running into the same problem, I tried to set the id on all the panels (also on the panelgroup) but it's not working, how did you manage to solving it?

@yangshun
Copy link
Author

Nothing special, I just added id fields to all the Panel components. If you provide a repro I can help to take a look.

@albertodeago
Copy link

I had the issue also specifying the ids. I actually solved using the API on the single Panel components

@bvaughn
Copy link
Owner

bvaughn commented Jul 15, 2023

Suggestion

I'm not sure if this is considered a bug, but I think it'd be useful to document somewhere that when doing dynamic/conditional rendering of <Panel>s, there's a potential issue with remounting and that stable ids can be passed in to prevent the issue.

This seems like a good suggestion. In general, I don't want to make id required because there are simple use cases where it's really not needed, but any time you have dynamic content going on, id and order are good props to have.

I'll add a note to the #FAQ section.

May also be nice to add a DEV-only warning if we detect the following scenario:

  • PanelGroup mounts with one set of Panels and later changes AND
  • Any Panel in the new set doesn't have an id prop

@bvaughn
Copy link
Owner

bvaughn commented Jul 15, 2023

FAQ updated in ddf346e. I'm going to give the DEV warning a bit more thought, but hopefully this at least helps others.

@bvaughn bvaughn closed this as completed Jul 15, 2023
@bvaughn
Copy link
Owner

bvaughn commented Jul 15, 2023

Dev warning added in #174 and published in 0.0.54


❤️ → ☕ givebrian.coffee

@yangshun
Copy link
Author

Thank you for this library! P.S. bought you some coffees ☕

@bvaughn
Copy link
Owner

bvaughn commented Jul 16, 2023

That was very kind of you! ❤️

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

No branches or pull requests

3 participants