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

Add isCollapsed boolean or size value to the Imperative API #71

Closed
josh-unwin opened this issue Jan 9, 2023 · 11 comments · Fixed by #73
Closed

Add isCollapsed boolean or size value to the Imperative API #71

josh-unwin opened this issue Jan 9, 2023 · 11 comments · Fixed by #73

Comments

@josh-unwin
Copy link

josh-unwin commented Jan 9, 2023

It would be handy to include either a status boolean or the current size of a panel in the Imperative API to use alongside the methods it provides.

Scenario:
You have a panel you want to toggle open or closed via another button component:

 import {
   ImperativePanelHandle,
   Panel,
   PanelGroup,
   PanelResizeHandle,
 } from "react-resizable-panels";
 
 const ref = useRef<ImperativePanelHandle>(null);
 
 const collapsePanel = () => {
   const panel = ref.current;
   if (panel.isCollapased) {
     panel.expand();
   } else {
     panel.collapse();
   }
 };
 
 <PanelGroup direction="horizontal">
   <Panel collapsible ref={ref}>
     left
   </Panel>
   <PanelResizeHandle />
   <Panel>
     right
   </Panel>
 </PanelGroup>
@bvaughn
Copy link
Owner

bvaughn commented Jan 9, 2023

I see why this could be convenient in some cases, but I'm not sure it's worth adding to the library since it's not new functionality. It seems pretty straight forward to implement this using the onCollapsed prop.

import {
  ImperativePanelHandle,
  Panel,
  PanelGroup,
  PanelResizeHandle
} from "react-resizable-panels";

const ref = useRef < ImperativePanelHandle > null;

const [isCollapsed, setIsCollapsed] = useState(false);

const collapsePanel = () => {
  const panel = ref.current;
  if (isCollapased) {
    panel.expand();
  } else {
    panel.collapse();
  }
};

<PanelGroup direction="horizontal">
  <Panel collapsible ref={ref} onCollapse={setIsCollapsed}>
    left
  </Panel>
  <PanelResizeHandle />
  <Panel>right</Panel>
</PanelGroup>;

@asterikx
Copy link

asterikx commented Jan 9, 2023

@bvaughn I was just about to ask the same question.

In the snippet you posted above, you assume that the panel is expanded to begin with, which is not necessarily true. When the panel is collapsed and the user reloads the page, isCollapsed is out of sync. It would require developers to derive the initial value of isCollapsed from the size state of the panel from local storage (developers should not rely on it as its encoding is internal to this library) or track it independently in local storage (which has great potential for sync errors). Please correct me if I have something wrong.

Btw, I really enjoy using your library, great work 👍🏻

@bvaughn
Copy link
Owner

bvaughn commented Jan 9, 2023

Ah, that's an interesting point. I had assumed onCollapse would be called for the Panel on load if it was initialized to size 0 but that doesn't seem to be the case.

Seems like maybe that's something that we could change.

bvaughn added a commit that referenced this issue Jan 10, 2023
Add getSize and getCollapsed to imperative Panel API
@bvaughn
Copy link
Owner

bvaughn commented Jan 10, 2023

panelRef.current.getSize() and panelRef.current.getCollapsed() available in v0.031

@bvaughn
Copy link
Owner

bvaughn commented Jan 10, 2023

Ah, that's an interesting point. I had assumed onCollapse would be called for the Panel on load if it was initialized to size 0 but that doesn't seem to be the case.

Even though the imperative API change has landed, I'm going to fix this too, because I think it's unexpected ~> #75

@asterikx
Copy link

asterikx commented Jan 10, 2023

I must admit, I would not have expected onCollapse to be fired on the first mount if the panel was initially collapsed. I think of methods with the prefix on as reacting to a change (usually triggered by a user interaction). Here it's the initial state, not a change. But then again, I don't think it will hurt anyone if onCollapse is called initially...

@bvaughn
Copy link
Owner

bvaughn commented Jan 10, 2023

I think it's important to synchronize any external state. How else would you know if the panel group had saved a previously collapsed state?

@asterikx
Copy link

Totally agree with you, sync errors should be avoided at all cost. It's possible to get the initial state using the ref passed to the panel via ref.current.getCollapsed(), no?
The onCollapse prop (and also ref.current.getCollapsed()) will provide the correct value only after the first render. It the panel contents fetch a lot of data, it would be ideal to delay the fetching until the user expands the panel. Not sure if you consider lazy mounting of panels as something that should be part of this package. I still think a hook called something along usePanelGroup that reads the panel group state directly from local storage (or cookie storage for SSR) and provides the correct state during the initial render might be something to consider in the future 👍🏻

@bvaughn
Copy link
Owner

bvaughn commented Jan 10, 2023

It's possible to get the initial state using the ref passed to the panel via ref.current.getCollapsed(), no?

Yes but not reliably bc of the server rendering approach to mount with an initial size of 1 for every panel. You would have to know to ignore the mount effect and read the second render- and I think this is too much an implementation detail to make for a good api.

The hook suggestion is an interesting one, but I would like to keep the api small if possible (avoid redundant ways of doing something) just to make maintenance more manageable for me.

@asterikx
Copy link

Yes but not reliably bc of the server rendering approach to mount with an initial size of 1 for every panel. You would have to know to ignore the mount effect and read the second render- and I think this is too much an implementation detail to make for a good api.

Good point 👍🏻

The hook suggestion is an interesting one, but I would like to keep the api small if possible (avoid redundant ways of doing something) just to make maintenance more manageable for me.

I have to admit that I'm pretty short on time at the moment. I don't need lazy loading at this moment, but possibly in the future. If my solution can be generalized, I would be happy to contribute back.

@bvaughn
Copy link
Owner

bvaughn commented Jan 10, 2023

I have to admit that I'm pretty short on time at the moment. I don't need lazy loading at this moment, but possibly in the future. If my solution can be generalized, I would be happy to contribute back.

Fair!

I think my suggestion at this point would be to use the onCollapse callback for this for the foreseeable future 😄

import { lazy } from "react";
import { Component } from "wherever";

const LazyComponent = lazy(Component);

function App() {
  const [shouldLoad, setShouldLoad] = useState(false);

  const onCollapse = (collapsed: boolean) => {
    // Wait to load panel contents until it's been shown (at least once)
    setShouldLoad(shouldLoad || !collapsed);
  };

  return (
    <PanelGroup autoSaveId="example" {...otherProps}>
      <Panel collapsible {...panelProps} onCollapse={onCollapse}>
        {shouldLoad && <LazyComponent />}
      </Panel>
      {/* ... */}
    </PanelGroup>
  );
}

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 a pull request may close this issue.

3 participants