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

Potential regressions with collapsed panels #220

Closed
NMinhNguyen opened this issue Nov 21, 2023 · 15 comments
Closed

Potential regressions with collapsed panels #220

NMinhNguyen opened this issue Nov 21, 2023 · 15 comments
Labels
wontfix This will not be worked on

Comments

@NMinhNguyen
Copy link

NMinhNguyen commented Nov 21, 2023

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.

0.0.53.mov

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?

0.0.61.without.minSizePercentage.mov

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%)

0.0.61.with.minSizePercentage.mov
@bvaughn
Copy link
Owner

bvaughn commented Nov 21, 2023

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

@NMinhNguyen
Copy link
Author

NMinhNguyen commented Nov 21, 2023

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

@bvaughn
Copy link
Owner

bvaughn commented Nov 27, 2023

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
Copy link
Owner

bvaughn commented Nov 27, 2023

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
Copy link
Author

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

@bvaughn
Copy link
Owner

bvaughn commented Nov 27, 2023

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
Copy link
Author

NMinhNguyen commented Nov 27, 2023

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
Copy link
Author

NMinhNguyen commented Nov 29, 2023

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
Copy link

Getting this in 0.0.63 also

@bvaughn
Copy link
Owner

bvaughn commented Dec 9, 2023

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

@bvaughn
Copy link
Owner

bvaughn commented Dec 10, 2023

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 bvaughn added the wontfix This will not be worked on label Dec 10, 2023
@bvaughn
Copy link
Owner

bvaughn commented Dec 10, 2023

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

@bvaughn bvaughn closed this as completed Dec 10, 2023
@NMinhNguyen
Copy link
Author

NMinhNguyen commented Dec 10, 2023

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
Copy link
Author

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

Expand.collapse.behaviour.mov

@bvaughn
Copy link
Owner

bvaughn commented Dec 10, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants