allotment icon indicating copy to clipboard operation
allotment copied to clipboard

Changing props after mount should have an effect

Open johnwalley opened this issue 3 years ago • 17 comments

Purpose

As a developer I expect changing a prop to have an effect, e.g. changing vertical from false to true would change the direction of the split.

Proposal

It's worth checking each prop to see if it's worthwhile supporting. Assuming it is then it might just be a case of re-initialising the internal state appropriately. We can even stand to lose some information, e.g. pane sizes.

A key requirement would be not to cause any child components to remount.

Acceptance criteria

  • [ ] As a developer I can change at least one prop and see the change reflected in the rendered component.

johnwalley avatar Sep 27 '21 13:09 johnwalley

I would like to be able to change the defaultSizes and have it relayout the pane sizes. I have some panes which are sometimes empty, sometimes not. I'm able to set their width correctly when the component loads, but after that I'm limited to using the reset method as documented in the storyboard stories, which doesn't take into account the defaultSizes prop.

I'm implementing something similar to the split view in the [https://codepen.io/](codepen.io editor), where each pane has a expanded and collapsed state.

I'm open to submitting a PR, but would appreciate any advice on how to approach it.

charles-stuart-appdetex avatar Jan 13 '22 22:01 charles-stuart-appdetex

Hi @charles-stuart-appdetex. That's a great feature idea!

I'd add a new method, e.g. resize. Eventually, I want the component to behave either in a controlled or uncontrolled manner with respect to each prop. My plan is to have it be uncontrolled for defaultSizes, i.e. it takes on the value on mount but from then on the component manages this state.

The challenge is all about working out which methods on the SplitView class to call. Something similar to the way defaultSizes gets used in the constructor but rather than calling addView call resizeView. Note that the constructor also handles the case where the sizes don't sum up to the size of the container and distributes the sizes proportionately.

I'm hoping to spend some time this weekend catching up on issues and this is one I anticipate tackling if you want to hold off writing the PR.

johnwalley avatar Jan 14 '22 14:01 johnwalley

That sounds great! I've really enjoyed using the component so far. It works really well. I used 'use-container-queries' to watch the width of the panels and track if they're "collapsed" or "open".

charles-stuart-appdetex avatar Jan 14 '22 21:01 charles-stuart-appdetex

@charles-stuart-appdetex I've had an initial attempt with #121 with included story.

It adds a resize method which takes an array of numbers. I'd be interested in any early feedback.

johnwalley avatar Jan 16 '22 00:01 johnwalley

Great, I'll check it out tomorrow. Thanks!

charles-stuart-appdetex avatar Jan 17 '22 02:01 charles-stuart-appdetex

I'm looking at it now. The resize seems to mostly behave how I would expect it to. It seems like with defaultSizes that I can set a width to 0 and then it'll set the width of that pane to the minSize I've specified. Using the storyboard, if I set a minSize to 20 and then resize([0, 1]), it sets the first pane to a width greater than 20. I'd like it to set it to the minSize or 0 if there is no minSize set.

I could be wrong, but from my understanding it should always respect the minSize, and also the widths you specify are basically a ratio of how to allocate the available width.

charles-stuart-appdetex avatar Jan 18 '22 00:01 charles-stuart-appdetex

Makes sense. I'll revisit at the weekend 👍. And thanks for the detailed feedback, it's appreciated.

I'm also considering adding a hide/show behaviour to panes. I've not thought it through yet but it could just be a prop on the pane, e.g. visible.

johnwalley avatar Jan 20 '22 23:01 johnwalley

I've had a look into this and here's my attempt at explaining the current behaviour.

With no minSize or maxSize set then defaultSizes or resize() will behave as though the values are ratios (a bit like flexbox).

However, the values are very much treated as pixel dimensions. They get clamped between minSize and maxSize before performing any layout calculations. For example, [0,1] with minSize=100 will effectively become [100,100] which, assuming space is distributed evenly will give a 50% split.

This does suggest giving up on the idea that the values are in pixels and will always be treated as, basically, percentages. See Split.js size prop as an example.

I think this is the way to go but I want to see how it interacts with the functionality which would let you set a fixed pixel size for a pane. For example, a two pane layout, where the left pane is a fixed width.

I'll keep the resize() PR open and track progress there.

I'm also working on #137 which adds a visible prop to panes. Effectively giving them zero size.

johnwalley avatar Jan 23 '22 18:01 johnwalley

Your explanation makes sense. I do think there are times where you might want to specify pixels values, ex: you want to collapse a panel to its minimized state, or, alternatively, if you want to open/expand a panel to a fixed to properly display content which is a fixed width.

On the other hand, it also definitely seems like you would want to be able to just tell it to divide up the available sizes based on a ratio, i.e. 33% for three, or 50% for one panel and 25% for two other panels if one panel needs more space.

charles-stuart-appdetex avatar Jan 26 '22 22:01 charles-stuart-appdetex

I haven't forgotten about this issue, but I'm still not sure how to move it forwards 🤔.

It's not obvious to me how to reconcile percentages and absolute sizes and also respect min and max sizes and not be confusing to users. I'll try to come up with some ideas.

johnwalley avatar Feb 08 '22 17:02 johnwalley

I've been writing a component, and I've been setting flex-basis, flex-grow, min-width with React, and then with CSS I set some additional default flex properties. flex-basis gets set with the absolute width that the user drags the panel resizer to, but then if you resize the viewport, etc. since you're setting flex-basis instead of width it behaves more like a percentage. When I want to maximize a panel, I set that one to flex-grow: 1 and set the others to flex-grow: 0, flex-basis: [minWidth]. For equivalent widths, they all get flex-grow: 1.

Then additionally there is some code to track if the flex-basis value is the min-width, and if so, it locks it to that width.

I'm modeling it after the split panels on CodePen, but I'm not supporting vertical splits as it's not a requirement for our app.

Anyways, I thought I'd share in case using mostly flex box would also help with reconciling percentages + absolute sizes, etc

charles-stuart-appdetex avatar Feb 09 '22 14:02 charles-stuart-appdetex

I can't seem to change the props for onChange.

If I dynamically add a new pane, I want to change the onChange function to be able to act on the new pane.

Currently the onChange function is saved on first render and any updates, or passing useCallback function won't update the internal onChange function. So it's impossible to act on the new state.

Only way to make it work now was to do the following

  const reducer = useCallback((state, action) => {
    switch (action.type) {
      case "openSideDrawer":
        return {...state, sideDrawer: true}
      case "openBottomDrawer":
        return {...state, bottomDrawer: true}
      case "closeSideDrawer":
        return state.sideDrawer ? {...state, sideDrawer: false} : state
      case "closeBottomDrawer":
        return state.bottomDrawer ? {...state, bottomDrawer: false} : state
      default:
        throw new Error()
    }
  }, [])

  const [state, dispatch] = useReducer(reducer, {sideDrawer: false, bottomDrawer: false})

  const onChangeBottom = useCallback((panes) => {
    if (panes[panes.length - 1] === 0) {
      dispatch({type: "closeBottomDrawer"})
    }
  }, [])

  const onChangeRight = useCallback((panes) => {
    if (panes[panes.length - 1] === 0) {
      dispatch({type: "closeSideDrawer"})
    }
  }, [])
  
  return <Allotment vertical onChange={onChangeBottom}>
        <Allotment.Pane>
          <Allotment onChange={onChangeRight}>
            <Allotment.Pane>
               Main pane
            </Allotment.Pane>
            <Allotment.Pane snap>
               Right pane
            </Allotment.Pane>
            {state.sideDrawer && <Allotment.Pane snap>
                Right drawer
            </Allotment.Pane>}
          </Allotment>
        </Allotment.Pane>

        {state.bottomDrawer && <Allotment.Pane snap>
          Bottom drawer
        </Allotment.Pane>}
      </Allotment>

benoist avatar Feb 22 '22 08:02 benoist

Hi @benoist. Sorry I missed this! I've created #236 and will address. Thanks for reporting 👍

johnwalley avatar Apr 11 '22 18:04 johnwalley

Hi @charles-stuart-appdetex. Thanks for the detailed information. I've added a preferredSize prop to panes which takes a percentage value. Should be straightforward to expand this to minSize and maxSize. Ultimately, the underlying VS Code implementation does describe something very flexbox like. Hopefully, I can expose all of that through the React interface eventually.

johnwalley avatar Apr 11 '22 18:04 johnwalley

Hi @johnwalley, Is it possible to have the onReset prop change too? Thanks

Kaishley avatar Nov 23 '22 06:11 Kaishley

Is there any update on this? I would like to conditionally change the props passed to Allotment.Pane, but the updated props doesn't re-render the pane.

~~In my case, I would like to disable a pane in some conditions, so I need add the maxSize props conditionally. Any suggestions?~~

I get the maxSize worked, but still have problem in preferredSize.

<Allotment.Pane minSize={40} {...conditionProps} >
... 

I set preferredSize to 50% for each pane then call ref.current.reset, where is wrong? A code sandbox example here

After checking, it seems the preferredSize is not compatible with minSize? If I comment out minSize, it would work. Otherwise, it will not. But double click on the sash will always work, any thoughts?

MoonSulong avatar Mar 28 '23 18:03 MoonSulong

I have noticed this being an issue when changing the vertical prop. A workaround is to force the Allotment element to fully re-render whenever the vertical prop changes. A simple way to do this might be to set the key on the Allotment element to the same value as the vertical prop, e.g.

<Allotment key={vertical} vertical={vertical} />

This does mean that the state is fully reset when swapping between horizontal and vertical. I would not expect this method to work well for other props for this reason.

I think it would make sense for Allotment to be able to handle this case, though.

erlend-amdal-adsk avatar Apr 03 '24 07:04 erlend-amdal-adsk