bvaughn / react-resizable-panels

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

Support pixel values for sizes #78

Closed viveleroi closed 1 year ago

viveleroi commented 1 year ago

We need to set specific pixel size values for the default, min, and max sizes for a panel. Please add support for pixel values, like 200px

viveleroi commented 1 year ago

I thought I had searched closed issues. Looks like you're not going to add that. Unfortunately it's a deal breaker for us so I'll look for alternatives. Thanks

sabbirzzaman commented 1 year ago

@viveleroi Did you find out any alternative package?

bvaughn commented 1 year ago

In case others find this issue rather than the original– this comment has an example of how to set pixel sizes: https://github.com/bvaughn/react-resizable-panels/issues/47#issuecomment-1368108429

The thread also explains the rationale for why I don’t plan to add support for that to the library directly.

viveleroi commented 1 year ago

@sabbirzzaman Not that fully satisfies our requirements, but part of the reason is that we have an annoying use case where one of the panels can be "unpinned" meaning it becomes position: absolute and the other panels "fall behind it". That panel can then be expanded/collapsed, but over the the panels. No one has support for that.

bvaughn commented 1 year ago

I think I have a possible solution for the pixel-based constraints; see https://github.com/bvaughn/react-resizable-panels/pull/176


❤️ → ☕ givebrian.coffee

viveleroi commented 1 year ago

Your timing is oddly perfect, I just pulled this effort out of my stale branches and have been finalizing it for our app. We're ditching pinned/unpinned stuff for now but I'm eager to try the pixel stuff. Will report back

bvaughn commented 1 year ago

Glad to hear. I hope it works for you.

There are some limitations but I think I settled on a decent balance between ease-of-use and flexibility. I just needed a big chunk of time to sit down and think about it.

viveleroi commented 1 year ago

I'm not sure if I should reply here or the PR. I'm glad you were able to add this feature, sounds like a lot of people will benefit. For our specific needs though, I don't think it'll work. My current approach is to use a useWindowResize hook from use-hooks-ts to get the width/height of the viewport and using our intended default/min/max pixel height, convert that into a percentage. From my testing so far that works well enough.

Looking at this new feature, I see two reasons this won't work out for us.

  1. The position prop seems like it only can apply to one panel at a time? That would probably be fine in most of our use cases but we do have cases where there are 3 panels: left, mid, right. The left/right are "drawers" that are 300px. I'd need to configure the pixel sizes of both.
  2. It's defined at the panel group level whereas the current default/min/max sizes are defined at the panel level. The above left/mid/right panels I mentioned are defined by each "submodule" of our app. Most are left/mid, some have all three, but they're defined at the submodule level so that future modules aren't locked into this format. Because of this, the components are passed as children to a Submodule component - which currently would make it a poor place to define the sizes for each panel because we don't "know" what panels are included. I could rework my components to solve this, of course.

Please let me know if I missed anything.

bvaughn commented 1 year ago

Thanks for the feedback! I'm glad you found a solution that's working for you 😄

Just a couple of thoughts:

The position prop seems like it only can apply to one panel at a time?

Yes, that's a limitation of the new usePanelGroupLayoutValidator hook– but not of the validateLayout prop itself. If your group had more than 3 panels, you could provide a custom validateLayout method that constrained more than one.

It's defined at the panel group level whereas the current default/min/max sizes are defined at the panel level.

I considered defining the new API at the level of the panel rather than the group, but it didn't work from an internal standpoint. Deltas trimmed off of (or added to) one panel need to be distributed between the other panels. I don't think that is something the panel group itself can do– at least not flexibly enough as a custom layout.

I wonder if it would be helpful for a use case like yours (or just in general) for the custom validation function to also receive the set of panel configurations for the group, e.g.

function validateLayout({
  availableHeight,
  availableWidth,
  nextSizes,
  panels,
  prevSizes
}: {
  availableHeight: number;
  availableWidth: number;
  nextSizes: number[];
  panels: Array<{
    collapsedSize: number;
    collapsible: boolean;
    defaultSize: number | null;
    maxSize: number;
    minSize: number;
  }>;
  prevSizes: number[];
}): number[] {
  // Compute and return an array of sizes
  // Note the values in the sizes array should total 100
}

Maybe that would make it easier for a centralized validation function to work with more "distributed" panels?

I guess it probably wouldn't, since the "panel data" this library tracks still isn't pixel based.

bvaughn commented 1 year ago

Thinking about this again today. I believe an API like this would probably work better for your use case:

type PanelProps = {
  collapsedSize?: number;
  defaultSize?: number | null;
  maxSize?: number;
  minSize?: number;

  // Default to "relative" (or percentage based)
  // if "static" all of the above size values should be treated as pixel based
  units?: "relative" | "static"; 

  // Other props...
}

type PanelGroupProps = {
  // Default implementation would respect "relative" and "static" configurations
  // but a custom implementation could still be provided if you wanted more control
  validateLayout = (
    groupSizePixels: number,
    panels: PanelData[],
    nextSizes: number[],
    prevSizes: number[]
  ) => number[];

  // Other props...
}

I'm not sure if I can make this API work without a ton of complexity, but it might be possible. It would probably be easier to use an API like this too, if I could make it work.

For instance, if you wanted a horizontal group with pixel-based constraints on the first and last panel only, you could do something like this...

 <PanelGroup direction="horizontal">
   <Panel minSize={150} maxSize={200} units="static">
     150-200 px
   </Panel>
   <PanelResizeHandle />
   <Panel>...</Panel>
   <PanelResizeHandle />
   <Panel>...</Panel>
   <PanelResizeHandle />
   <Panel minSize={150} maxSize={200} units="static">
     150-200 px
   </Panel>
 </PanelGroup>

Maybe I wouldn't even need to expose validateLayout publicly if the default implementation could support the above type of group...

viveleroi commented 1 year ago

That looks like it would work well for our case. If you can achieve it I'll be happy to test it out.

bvaughn commented 1 year ago

My initial spike for this is very promising. It's kind of a complicated change, but I think it's something I should be able to land. Just need to write a lot of new integration tests.

https://react-resizable-panels-7i3lk12pg-bvaughn.vercel.app/examples/pixel-based-layouts

bvaughn commented 1 year ago

Okay. I think this change will ship, sometime in the next couple of days. If you end up using it and finding it helpful, I'd love to know.

viveleroi commented 1 year ago

Please let me know when it goes out and I'll give it a shot!

bvaughn commented 1 year ago

@viveleroi Published as 0.0.55 -> https://github.com/bvaughn/react-resizable-panels/blob/main/packages/react-resizable-panels/CHANGELOG.md#0055

viveleroi commented 1 year ago

I'm not sure what's wrong for me but after installing v0.0.55 and trying your example code I'm seeing two unexpected issues:

  1. Invalid Panel minSize provided, 300
  2. I see no units declaration in PanelProps in the node_modules\react-resizable-panels\dist\declarations\src\Panel.d.ts file.

Has the API changed since you explained it, is there a deployment error, or have I missed something?

<Panel
  className='flex'
  collapsedSize={36}
  collapsible
  defaultSize={300}
  minSize={300}
  onCollapse={onCollapse}
  ref={ref}
  units='static'>
  {content}
</Panel>
bvaughn commented 1 year ago

Hey @viveleroi 👋🏼 Sorry for the confusion, but the API that shipped in 55 changed a bit from the initial sketch I left above. The "units" prop actually needs to go on the PanelGroup for now. (You can see an example of the API here.) The warning you're getting is because the group thinks you're setting the default Panel size to 300%

The API I'm working on in #182 is more flexible and will allow using mixed percentage and pixel layout constraints.

viveleroi commented 1 year ago

Ah, I was looking at the documentation you had linked and missed the changelog. I was able to get it working.

I'm glad you mentioned support for mixed pixels/percents because that's exactly what we need in this case. Our default/min sizes are 300px but our max size is 60%.

I am noticing that onCollapse isn't being invoked anymore. The collapse functionality seems to be working just fine though - below the minSize it eventually collapses to the defined width but the callback is never called after updating. Only happens when units='pixels', it's called correctly when using percentages.

bvaughn commented 1 year ago

RE onCollapse - #183

viveleroi commented 8 months ago

Just updated to 1.0 and realized the pixel work you did was stripped out. Why was that dropped? I assume it's never to return?

bvaughn commented 8 months ago

Why was that dropped? I assume it's never to return?

@viveleroi I mentioned this in the version 1.0 PR and it's listed on 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 have no plans to re-add that functionality. This library is a side project that I do in my spare time, not a paid effort. As such, I chose to do fewer things well and reduce the amount of time I spend chasing down edge cases.

If a client of this library has a use case for pixel constraints and wants to talk about paying me to build that, I may reconsider. Else this is likely a final decision.