bvaughn / react-resizable-panels

https://react-resizable-panels.vercel.app/
MIT License
3.58k stars 124 forks source link

Duplicate panel id throws AssertionError case #307

Closed DenizUgur closed 4 months ago

DenizUgur commented 4 months ago

I've added a test case for the issue described in #303. The test case is not perfect but it throws the AssertionError as expected so I hope this would help to track down the problem.

For future readers: Ensure you have unique id's for your PanelGroups as well, if rendering conditionally.

closes #303

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-resizable-panels ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 23, 2024 0:06am
bvaughn commented 4 months ago

I'm not sure what this test is showing, yet.

One thing I do notice is that– if you're dynamically rendering panels, this library asks that you specify both an id and an order prop to make sure the layouts map correctly (so when "bar" moves from being the 2nd panel, to the first, its layout is preserved). That's one of the warnings the test is surfacing.

bvaughn commented 4 months ago

If you're swapping this group between "vertical" and "horizontal" layouts, the easiest approach here would probably be to use a different id for these two cases (e.g. "group-vertical" and "group-horizontal")

DenizUgur commented 4 months ago

During my debugging I also gave a monotonically increasing number for order prop but that didn't do anything. Only thing that worked was ensuring outermost panel group had unique id.

bvaughn commented 4 months ago

During my debugging I also gave a monotonically increasing number for order prop but that didn't do anything. Only thing that worked was ensuring outermost panel group had unique id.

Yes, that wouldn't fix the error you're reporting but it is another thing you would need to address here (and the library warns about it in DEV mode)

DenizUgur commented 4 months ago

If you're swapping this group between "vertical" and "horizontal" layouts

Ah, so that was the issue.

And for the order prop, what would you do in nested groups? Each groups first panel should start with 0 or I should give an order in the order I render them (regardless of the group)

bvaughn commented 4 months ago

And for the order prop, what would you do in nested groups? Each groups first panel should start with 0 or I should give an order in the order I render them (regardless of the group)

The order should be their rendering order (or their order in the DOM)

Ideally I should be able to infer this, but it's awkward to do that with React's parent/child boundaries

bvaughn commented 4 months ago

Thanks for sharing this repro. I think it's helped me track down a bug and a gap in my unit test coverage. I think I have a fix in #309 so I'm going to close this PR.