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 values for sizes #78

Closed
viveleroi opened this issue Jan 12, 2023 · 21 comments
Closed

Support pixel values for sizes #78

viveleroi opened this issue Jan 12, 2023 · 21 comments

Comments

@viveleroi
Copy link

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

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

@viveleroi Did you find out any alternative package?

@bvaughn
Copy link
Owner

bvaughn commented Jan 17, 2023

In case others find this issue rather than the original– this comment has an example of how to set pixel sizes: #47 (comment)

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

@viveleroi
Copy link
Author

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

bvaughn commented Aug 6, 2023

I think I have a possible solution for the pixel-based constraints; see #176


❤️ → ☕ givebrian.coffee

@viveleroi
Copy link
Author

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

bvaughn commented Aug 6, 2023

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

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

bvaughn commented Aug 6, 2023

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

bvaughn commented Aug 8, 2023

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

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

@bvaughn
Copy link
Owner

bvaughn commented Aug 9, 2023

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

bvaughn commented Aug 9, 2023

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

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

bvaughn added a commit that referenced this issue Aug 13, 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.:
```tsx
 <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.
```ts
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](https://react-resizable-panels-git-panelgroup-layout-val-2424f0-bvaughn.vercel.app/examples/pixel-based-layouts)
@viveleroi
Copy link
Author

viveleroi commented Aug 18, 2023

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

bvaughn commented Aug 18, 2023

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

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

bvaughn commented Aug 18, 2023

RE onCollapse - #183

@viveleroi
Copy link
Author

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

bvaughn commented Jan 11, 2024

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.

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

No branches or pull requests

3 participants