bvaughn / react-resizable-panels

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

.isCollapsed() seems very unreliable? #325

Closed oskarengstrom closed 3 months ago

oskarengstrom commented 3 months ago

I'm calculating the collapsedSize of a panel to enable px-based values, and running into some issues related to that. It seems, when using this hook to supply a value to collapsedSize .isCollapsed() is always returning false. I can run .collapse() successfully on a ref of the panel, but then .isCollapsed() returns false.

First my guess was that the number had too many decimals (there seems to be an issue with more than 10 decimals), but toFixed() is not solving my OG issue.

So then I was guessing - if the fact that my hook returns one number on first render, and then another on second, could be the issue? This seems more likely.

I will continue testing on my end, but would be nice to hear if anyone has any ideas on what might be wrong?

(v2.0.13)

bvaughn commented 3 months ago

Please share a repro. It's really hard to help trouble shoot issues where there's no shared code.

I suggest you also check out #265 and #264 since they seem to be related. Generally speaking, I have decided not to support pixel based constraints, so what you're doing might just be out of scope for this library, but I can't tell without seeing a repro.

oskarengstrom commented 3 months ago

Here's a repro: https://codesandbox.io/p/sandbox/react-resizable-panels-forked-pydfkc

As you can see the left panel (using the minSize-hook to generate the value for collapsedZise) isn't expanding, but the right (which uses an integer value) can indeed expand.

bvaughn commented 3 months ago

I recorded a Replay of the repro you provided, and I can see where the isCollapsed method is returning something other than what you're expecting. This is happening because the panel's actual size (3.8764385221) is not the same as its collapsed size (3.8764385221078133). (So it's basically a precision issue.)

I try to account for potential precision issues within the library's own methods, but this case kind of falls between the cracks a bit, because useMinSizePx is passing a more precise value than the library expects. (Things like this are one of the reasons I decided not to support pixel based constraints.)

I guess I could account for this by using a fuzzy comparison in the is-collapsed/is-expanded helper methods.

bvaughn commented 3 months ago

2b0ff1a should guard against high precision values from code like the example you shared. It has been released in version 2.0.14.


❤️ → ☕ givebrian.coffee

oskarengstrom commented 3 months ago

Thanks a lot @bvaughn! 🙏

I did notice that the props onCollapse and onExpand stopped working now 🤷‍♂️ Updated the repro: https://codesandbox.io/p/sandbox/react-resizable-panels-collapsedsize-bug-pydfkc?file=%2Fsrc%2FApp.js%3A42%2C37

bvaughn commented 3 months ago

The number of edge cases with high precision values are part of the reason I chose not to support pixel based constraints.

2.0.15 should fix the callback issue. If you uncover anything else though, you'll need to submit a PR. I don't think I have the mental energy to try to support this use case 😅 Thanks for understanding.


❤️ → ☕ givebrian.coffee

bvaughn commented 3 months ago

Think I accidentally re-opened this one so I'm going to close it now. We can talk more on a new issue if anything else crops up.