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

Support pixel-based panel constraints #176

Merged
merged 20 commits into from
Aug 13, 2023
Merged

Conversation

bvaughn
Copy link
Owner

@bvaughn bvaughn commented Aug 6, 2023

Relates to issues #46, #47, #51, #78, #114, #128, #141

This PR adds a new prop (units) to PanelGroup. This prop defaults to "percentage" but can be set to "pixels" for static, pixel based layout constraints.

This can be used to add enable pixel-based min/max and default size values, e.g.:

 <PanelGroup direction="horizontal" units="pixels">
   {/* Will be constrained to 100-200 pixels (assuming group is large enough to permit this) */}
   <Panel minSize={100} maxSize={200} />
   <PanelResizeHandle />
   <Panel />
   <PanelResizeHandle />
   <Panel />
 </PanelGroup>

Imperative API methods are also able to work with either pixels or percentages now. They default to whatever units the group has been configured to use, but can be overridden with an additional, optional parameter, e.g.

panelRef.resize(100, "pixels");
panelGroupRef.setLayout([25, 50, 25], "percentages");

// Works for getters too, e.g.
const percentage = panelRef.getSize("percentages");
const pixels = panelRef.getSize("pixels");

const layout = panelGroupRef.getLayout("pixels");

See the docs for more: .../examples/pixel-based-layouts

@vercel
Copy link

vercel bot commented Aug 6, 2023

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 Aug 13, 2023 1:35am

@bvaughn bvaughn merged commit ed8d712 into main Aug 13, 2023
5 checks passed
@bvaughn bvaughn deleted the PanelGroup-layout-validator-alt branch August 13, 2023 18:20
@bvaughn bvaughn mentioned this pull request Dec 13, 2023
7 tasks
@lacymorrow
Copy link

Just wanted to mention that it seems like using min/max widths on the ResizablePrimitive.Panel allows for constraining the width using pixels.

Granted, I am using the shadcn implementation here: https://ui.shadcn.com/docs/components/resizable

@piszczu4
Copy link

Just wanted to mention that it seems like using min/max widths on the ResizablePrimitive.Panel allows for constraining the width using pixels.

Granted, I am using the shadcn implementation here: https://ui.shadcn.com/docs/components/resizable

But if you set min-width on Panel then the resizing in the background still works (even if does not have an impact) and when you want to increase the size again, it does not respond until you revert the changes (your mouse moves away from the resize handle). Is it possible to fix it somewhow?

@lacymorrow
Copy link

But if you set min-width on Panel then the resizing in the background still works

Can you say more about this? I'm not sure what you mean by "resizing in the background", do you mean programmatically?

@zac18992
Copy link

zac18992 commented Jan 18, 2024

@lacymorrow

For example, first apply the following css or style to a panel:

max-width: 500px;

Then use the resize handle to expand the width of the panel. Once the panel width reaches 500px, the element will (correctly) not change in size. However, as you drag past 500px, or if you continued to attempt to drag the resize handle to increase the size, the actual width of the panel (within the library) is > 500px. You could check this by logging from the onResize handler.

In summary, if you drag to 650px, the visual element correctly stops expanding at 500px. However, when you drag to decrease the size, you will have a 150px dragging delay before the element starts reducing in width.

@bvaughn This one hurdle could be resolved by having the ability to programatically interrupt resizing a panel. This way, for example we could have something like:

shouldResize={({ width, height }) => (width <= 500)}

I don't know if this is simple or complex, as this would depend on the implementation within the library.

Even with the library currently as is, I could envisage this being solvable - by calling the imperative resize(size: number) on the back of onResize to resize the panel back down to the maximum/minimum width/height. But if using percentages like me, this would require a conversion of the percentage size to pixels. This feels a little messy though so not the ideal approach.

@lacymorrow
Copy link

Bravo on illustrating the issue.

For folks in the future: max-width can improve the experience, but not solve the problem entirely.

@bvaughn
Copy link
Owner Author

bvaughn commented Jan 18, 2024

@zac18992 I've intentionally decided not to support pixel constraints, as mentioned in the README:

Can panel sizes be specified in pixels?

No. Pixel-based constraints added significant complexity to the initialization and validation logic and so I've decided not to support them. You may be able to implement a version of this yourself following a pattern like this but it is not officially supported by this library.

I haven't thought through the implications of a prop like shouldResize but I don't think it would have any use with percentage based constraints, so I would be pretty reluctant to add it.

@bvaughn
Copy link
Owner Author

bvaughn commented Jan 18, 2024

You're right to call out the downside of the approach discussed above though! 👍🏼

@zac18992
Copy link

@piszczu4 @lacymorrow This issue is very important to my use case. I believe I have found my own solution to prevent this, based on the above, but I will try to finalise it before sharing an example, to check if there are any other issues.

@bvaughn I do believe that shouldResize (amongst other props) would give greater control, not only solving this issue, but wider use cases that are not tied specifically to percentage or pixel use cases. This prop alone could even prevent resizing based on conditions which don't rely on sizing of panels, such as displayed contents or state.

However, I'm not saying I expect you to implement this. Of course you need to limit the scope so you can be productive. Thanks for replying!

@bvaughn
Copy link
Owner Author

bvaughn commented Jan 18, 2024

Thanks for understanding, @zac18992 🙇🏼

I will try to finalise it before sharing an example, to check if there are any other issues.

Happy to link to your solution in the FAQ as well!

@mrmattrc
Copy link

While I understand the desire to keep things simple and easy to work on, removing support for pixels is major pain point for a SAAS project my team is working on. We've resorted to downgrading to v0.0.63 so we can continue using pixel and percentage values. However, we've recently seen some runtime errors (Cannot read properties of undefined (reading 'collapsedSizePercentage'), so we are considering our options.

Thankfully, I happened to look through this issue when I did and found @zac18992's solution. I'm hopeful that your solution will work for our use case and maybe even address the runtime errors as well. I'll give it a shot and report back.

@bvaughn
Copy link
Owner Author

bvaughn commented Jan 18, 2024

@mrmattrc I'm sorry to hear that your project was negatively impacted by this decision. This is a side project for me though, done pro bono, and I have to keep scope manageable if I want to continue to support the project at all.

If this is a commercial use case, we could always chat about the possibility of me doing some for-hire work.

@zac18992
Copy link

zac18992 commented Jan 19, 2024

My use case is always to use responsive layouts (with percentages) but requires min and max values in PX for panels.

Here is an example of how I got this to work with the library as it currently is.

This rectifies the issue and prevents panels from extending past their designated max/min span, which is set using an inline style. It may have some bugs or performance considerations. I haven't explored using the library with purely PX values, but I think the above covers the majority, if not all of those use cases. React Context could be used for passing the internal values for the min/max mechanism, if preferred.

Like I said, this should probably be in the library and would probably be a lot easier to implement, and more performant, if done so. But sounds like @bvaughn needs convincing and then someone needs to create a PR :)

Hope this helps some people!

@bvaughn
Copy link
Owner Author

bvaughn commented Jan 19, 2024

Code examples are good, if they've been vetted. I'm willing to link to them in the FAQs if they'll help others (again, only if they've been tested and seem to work well).

But I want to set expectations here regarding feature support for this. I believe that supporting pixel based constraints in this library would be a lot of work (both to implement initially and to maintain over time) and I am not willing to do it pro bono. If someone wants to talk about some sort of for-hire arrangement then I'm willing to consider it but I'm not really interested in anything short of that.

I hope you'll all understand that my free time is limited, and it's really important to me to have a good balance between sharing libraries like this and not burning myself out trying to add features for other people (particularly in support of for-profit use cases).

@zac18992
Copy link

zac18992 commented Jan 19, 2024

I haven't looked at all the edge cases. But from first glance, I do believe the support could be simpler than it seems. Especially as it can already be fudged externally using the existing version. It could maybe be added with a prop allowing for control over whether a resize can happen or not. Or probably using a few other approaches as well.

Anyway, I'm currently happy with what I have. If I want to add it to the library, maybe for performance reasons, then I will just fork it and share, as it sounds like this is not something that will be considered.

Thanks for all your great work on this library. I really appreciate it!

@bvaughn
Copy link
Owner Author

bvaughn commented Jan 19, 2024

Having tried to add support already, I think you're under estimating the effort for this. You don't just need to make per-panel decisions at the time of a resize / mouse event, you also need to listen for size changes and re-evaluate panel constraints for the entire group (which can get really complicated).

That being said, fork and share sounds like the way to go.

@zac18992
Copy link

zac18992 commented Jan 19, 2024

I can totally see that! Like I said, I haven't looked at all the edge cases, and am not pushing any agenda as I believe I have it working as I desire already :). In my case, I believe the panel constraints are handled by the existing percentage based logic.

@mrmattrc
Copy link

@bvaughn, re-reading my comment, I feel like I was more harsh than I intended. It's great having a library to handle this functionality, and I really appreciate all your work! We are a small and early-stage start-up though, so we're pretty restricted as far as sponsoring or for-hire work.

It's not perfect, but it looks like the solution suggested here should work pretty well for us! We are running into this minor issue:

In summary, if you drag to 650px, the visual element correctly stops expanding at 500px. However, when you drag to decrease the size, you will have a 150px dragging delay before the element starts reducing in width.

I agree with @zac18992 that adding something like a shouldResize prop seems like it would help. And adding documentation for this use case would also be great!

@zac18992
Copy link

zac18992 commented Jan 20, 2024

@bvaughn, re-reading my comment, I feel like I was more harsh than I intended. It's great having a library to handle this functionality, and I really appreciate all your work! We are a small and early-stage start-up though, so we're pretty restricted as far as sponsoring or for-hire work.

It's not perfect, but it looks like the solution suggested here should work pretty well for us! We are running into this minor issue:

In summary, if you drag to 650px, the visual element correctly stops expanding at 500px. However, when you drag to decrease the size, you will have a 150px dragging delay before the element starts reducing in width.

I agree with @zac18992 that adding something like a shouldResize prop seems like it would help. And adding documentation for this use case would also be great!

Hey @mrmattrc @lacymorrow @piszczu4 check the code sample in my above comment. It could be of use :)

#176 (comment)

@bvaughn
Copy link
Owner Author

bvaughn commented Jan 20, 2024

@mrmattrc No worries! 😄 I appreciate your comment though!

I'll let the shouldResize idea bounce around my head a bit. I'm concerned that even that could introduce complexity though, when it comes to enforcing group panel constraints. (How should this library handle the case where a panel "refuses" a resize action that's required in order to satisfy another panel's constraints? What if one panel "accepts" a resize, the sibling panel (the one on the other side of the resize bar) doesn't?)

Report back with the approach that's being discussed above. If it works out well for you all then I can add it to the FAQs.

@rortan134
Copy link

In case anyone is looking for this here's how I managed to do it w/ shadcn. Some glitches still may happen though and it most likely doesn't handle every edge case.

codesandbox

@DawidWraga
Copy link

The README says that panel sizes cannot be specified in pixels and indeed units="pixels" does not work.

So the fact that this PR was merged, and these docs exist is confusing

@bvaughn
Copy link
Owner Author

bvaughn commented May 11, 2024

@DawidWraga This PR and the CI build for this branch are 9 months old. Pixel-based constraints was something I tried and abandoned when it became clear that there were too many edge cases for me to support them correctly. (This is a side project for me. No one is paying me to build those features.)

If you want to use an older release that had pixel based constraints, you can.

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

Successfully merging this pull request may close these issues.

7 participants