bvaughn / react-resizable-panels

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

Potential regressions with collapsed panels #220

Closed NMinhNguyen closed 10 months ago

NMinhNguyen commented 10 months ago

For context, I tried updating from 0.0.53 to 0.0.61 and noticed potential regressions.

Using 0.0.53, I have a panel that's collapsed by default (defaultSize={0} + collapsed), and using ImperativePanelHandle I'm able to expand/collapse it, and it'll remember the last committed values. Note that I'm also using resize(40) if it's the first expansion.

https://github.com/bvaughn/react-resizable-panels/assets/2852660/2a1bfe4c-b492-4bb5-bdbf-fdd256f332a9

Following https://github.com/bvaughn/react-resizable-panels/blob/main/packages/react-resizable-panels/CHANGELOG.md#example-migrating-panels-with-percentage-units, I updated defaultSize to defaultSizePercentage, getCollapsed to isCollapsed, and onCollapse to onExpand, however using 0.0.61, it seems like it forgets its previous size before it was collapsed?

https://github.com/bvaughn/react-resizable-panels/assets/2852660/deed8e30-67c7-4a80-a140-b91b3c810d53

I then did a bit of looking through the code, and I believe in 0.0.53 minSize was defaulted to 10, but the default has since been removed. So I added minSizePercentage={10}, but it still doesn't behave like it used to - that is, it expands to minSizePercentage instead of the last size (40%)

https://github.com/bvaughn/react-resizable-panels/assets/2852660/3c2ad8cc-f4f9-4732-94bd-e6f9f75ca5c1

bvaughn commented 10 months ago

If you share a Replay or as least a Code Sandbox of the bug, I’ll take a look.

NMinhNguyen commented 10 months ago

I did share 2 Code Sandboxes, they're hidden behind hyperlinks for 0.0.53 and 0.0.61 respectively

bvaughn commented 10 months ago

My apologies @NMinhNguyen. I didn't realize those were Code Sandbox links. I thought they were links to my own release notes or something.

bvaughn commented 10 months ago

That being said, both repros you linked me to seem to work the same in terms of collapse/expand.

I suspect you might be triggering an edge case somehow that I'm not able to trigger, where the last remembered size is 0 so it re-"expands" back to 0:

// Store size before collapse;
// This is the size that gets restored if the expand() API is used.
panelSizeBeforeCollapseRef.current.set(panelData.id, panelSizePercentage);
// Restore this panel to the size it was before it was collapsed, if possible.
const prevPanelSizePercentage = panelSizeBeforeCollapseRef.current.get(panelData.id);

That doesn't happen for me for some reason though.

I can add a guard against it though.

NMinhNguyen commented 10 months ago

No problem and thanks for looking into this. I'll try to record a replay in the coming days

bvaughn commented 10 months ago

I believe 7a3a778 should prevent the issue you're describing. Try version 0.0.62 when you get the chance and see if that fixes things for you.

NMinhNguyen commented 10 months ago

I suspect you might be triggering an edge case somehow that I'm not able to trigger, where the last remembered size is 0 so it re-"expands" back to 0:

That's referring to the video without minSize, right? I think once I specified it as 10%, the resizing to 0% issue went away.

I'll try to reproduce the latest video using 0.0.62 and let you know. Thanks again!

NMinhNguyen commented 10 months ago

I updated the 0.0.61 Sandbox to use 0.0.63 and recorded a replay: https://app.replay.io/recording/react-app--388118c2-aa91-4623-9bc7-d2c3d9f184e6

alexolivier commented 10 months ago

Getting this in 0.0.63 also

bvaughn commented 10 months ago

Thanks, @NMinhNguyen. That will be very helpful when I get time to look at this.

bvaughn commented 10 months ago

Thank you for sharing the Replay.

Looking at the Replay, I believe it shows things working as expected. Internally, I keep track of a Panel's size before collapse using a ref, panelSizeBeforeCollapseRef. That ref is only written do when the Panel.collapse() API is called and it's only read from when the Pane.expand() API is called. In the Replay you shared, you "collapse" the panel by drag-resizing it, so the ref is never written do. So when you re-expand it, it just re-expands to the min size.

It's not clear to me what (other than this) you'd expect to happen in this case? Are you expecting the panel to re-expand to its default size?

I think the behavior shown in v0.53 was actually a bug (at least relative to my expectations of how the library should work).

bvaughn commented 10 months ago

Going to close this issue since I don't plan to take action on it. Happy to continue talking here though.

NMinhNguyen commented 10 months ago

Personally, I think the 0.53 behaviour is more intuitive - I don't think it should matter whether collapse() API was explicitly called or whether the panel was collapsed as a result of a drag interaction. The new behaviour may appear to the user as if the last expanded size was forgotten. And the minSize merely determines the threshold where collapsing happens - I don't think the user would expect clicking on an expand button to expand to that.

Alternatively, if you have suggestions as to how I can bring back the old behaviour in user land, I'm happy to use that approach instead.

If you try this interaction using VS Code, you'll see that it matches the old behaviour. Sorry, I'm not able to record it right now but should be easy enough to repro.

NMinhNguyen commented 10 months ago

You can see the VS Code behaviour here, although my theme makes it hard to see the resize handle:

https://github.com/bvaughn/react-resizable-panels/assets/2852660/32452b4b-0180-47cf-bd5c-e4fb2a0d6828

bvaughn commented 10 months ago

Looks like VS Code remembers the most recent dragged size too, on some kind of debounce/throttle maybe so that it doesn't always end up being min size (the last non-collapsed size). If this matters to you, feel free to open a PR that adds this behavior (after #230 lands). I don't think I will add it myself.