ui: Update ResizablePanelGroup internals #1355
Conversation
It doesn’t work! But I’m going to try a PanelContext class.
Things are draggable again! BUT why even have this class when the panels could contain the data? Unsure…
This just makes for easier comparison to the base branch.
this is of course to be removed but I need to make a maybe- significant change and don’t want to lose it all
all the tests pass!!!! now for the skipped one
Maybe I want to clean these up somehow, why be inline? etc but who knows
| test<MyTestContext>('it adjusts to its container growing (default)', async function (assert) { | ||
| this.renderController.containerStyle = | ||
| 'max-width: 100%; height: 200px; width: 218px;'; | ||
| await renderHorizontalResizablePanelGroup(this.renderController); |
There was a problem hiding this comment.
assertion after this to show status quo
There was a problem hiding this comment.
Can you expand on this? I wouldn’t normally repeat assertions when I consider a case covered.
There was a problem hiding this comment.
Oh wait. Pff I thought the modifier changed the width of panel-0-content or something. My bad.
Yea doesn't seem to be a need to "repeat" this assertion. My point was if the width change from 200 to 240. Id want 200 to be asserted. Then 240 to be asserted.
...ges/boxel-ui/test-app/tests/integration/components/resizable-panel-group-horizontal-test.gts
Outdated
Show resolved
Hide resolved
| this.renderController.panels[1].lengthPx = 200; | ||
| this.renderController.panels[0].innerContainerStyle = | ||
| 'height: 180px; background: blue'; | ||
| await renderHorizontalResizablePanelGroup(this.renderController); |
There was a problem hiding this comment.
I think perhaps, this one makes sense to assert
...ges/boxel-ui/test-app/tests/integration/components/resizable-panel-group-horizontal-test.gts
Outdated
Show resolved
Hide resolved
| assert.hasNumericStyle('.panel-1-content', 'width', 67, 1); | ||
| }); | ||
|
|
||
| test<MyTestContext>('it adjusts to its container shrinking and growing', async function (assert) { |
There was a problem hiding this comment.
Actually, this test seems to want to test growing followed by shrinking.
I think its too generic and can be broken up.
- I dont think we need a test to display shrinking followed by growing (wdyt)
- double clicks should be separate test
- isnt the growing and shrinking handled in test before? so, maybe some portions of this test is slightly redundant
...ges/boxel-ui/test-app/tests/integration/components/resizable-panel-group-horizontal-test.gts
Outdated
Show resolved
Hide resolved
| }); | ||
| } | ||
| } | ||
| calculateLengthsOfPanelsWithoutMinLength(); |
There was a problem hiding this comment.
I recall this is the main calculation code that consolidates length and ratio and the panels. Its relatively complicated. And I know thereis some issues with this code bcos ratios are normalised as in between of adding and removing new panels. This will essentially, lead to ratios being morphed. For example, when going from 3 panels to 2 panels by removing panels and adding
It goes like (only considering ratios)
- 3 panels 0.2,0.4,0.4
- 2 panels 0.33, 0..66 (unregistered a panel)
- 1 panel 1 (unregistered a panel)
- 2 panel 0.769, 0.23 (registered a panel)
You can see that the ratios morphed maybe not for the better
In the spirit to be aware of this, can we separate this into a block of code where ratio calculation exist. Then we can maybe add test there in the future
There was a problem hiding this comment.
yes, I see what you’re saying. I’ll include something like this in the followup that addresses the ratios. Does this mean that maybe the old ratios before hiding should be tracked and restored when unhidden? Seems like an explosion of complexity is possible 😳
There was a problem hiding this comment.
It sounds like an explosion of complexity if its all inside the component. Tho, im also skeptical that you can prevent the ratios from being morphed if you commit to the principle of recalculate them every time you register or unregister a new panel.
we probably have to decide on a heuristic for this to be stable. But, I think its just hard to think about at its current state. For hiding and persistence, I didn't think you need to change the ratios. I thought you'd just store some new state in the local storage?
There was a problem hiding this comment.
I think this code should belong together in some sort of abstraction. wdyt
calculateLengthsOfPanelsWithMinLength,calculateLengthsOfPanelsWithoutMinLengthuses the intermediate ratios from registering to update the lengths onContainerResizecalculateRatiosupdates the intermediate ratios upon register, unregistering, moving the handles
tho im ok also not including these changes in this PR
|
I recall messing about freestyle usage stuff and the the resizable panel doesn't work exactly as expected. Do u experience this? |
Which part? The Thanks for the earlier feedback, I’ll address it today 😀 |
This was never attached to a container!
This refactors
ResizablePanelGroupand itsPanelandHandlechildren. I originally planned to fix a bug where the panels don’t resize when one has been hidden but the fix needs more attention, so I’ll make a followup PR when it’s ready.Bookkeeping via parallel
PanelContextarrays is gone:PanelandHandle${ResizeHandleElIdPrefix}-${this.args.orientation}-${id})idseen there is instead derived from the component’s position in the arrayPanelember-ref-bucketResizeHandleris nowHandleinternally,ResizeHandlewhen exported.@isHiddenfor a panel is now documented in Freestyle.Tests now exercise horizontal orientation too. I thought of combining the horizontal and vertical tests as they’re identical other than using the appropriate dimensions but that seemed harder to understand. That means any new tests for the component would need to be added to two files, so if people think combining them is a good idea, I’m open to that.