-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiResizableContainer] Allow collapsible panels #3978
Conversation
bfcf228
to
d3343ec
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_3978/ |
🎉 Great start! I think this will definitely meet Discover's needs and replicate a similar behavior that exists now. I'd love to see a version that allows the toggle button to be free-floating, meaning, it doesn't have to live in the DOM layout (possibly pushing content down). A somewhat static version could be just adding the button on focus that floats in the center where the original grab button was. For instance, we'd have these 4 states (the fourth showing the collapsed state that maybe always shows the uncollapse button). ^^ That solution will work well with applications that are ok keeping at least a 24px wide area dedicated to the collapsible button. However, we'd probably also need a solution for consumers who would like to provide their own toggle button anywhere in their UI that would then completely hide or show this panel (similar to like a flyout toggle). You could handle this by allowing One more enhancement could be to add a double-click behavior which will do the collapsing/uncollapsing behavior so that users don't have to hunt for the button itself. It would be a learned behavior, but that is ok since we're also providing the targetable button. |
Because the collapse functionality - defined on one or more panels - affects the other panel(s) in a group, I think the majority of the logic needs to move to the panel registry which is already responsible for orchestrating the group. Some additional concerns:
Reported widthsAs an example let's start with 3 panels all open and with 33% width.
Let's collapse the middle column. The total displayed width should remain the same even though A and C together still represent only 66% of the size.
Now resize column C larger
Numerically, what happened? Today, with un-collapsible columns, resizing C would take away from B. Collapsed, we cannot take away from B because visually it wouldn't adjust C's width, because B is already collapsed (this is not strictly true, as proportionally A would still shrink to accommodate C's increased size, but that feels really awkward from a user's perspective). Does it look over further and take away from A? What sizes are expected when we expand B again? Would any of those answers be meaningfully different if we had shrunk C instead of enlarging? |
I made a diagram to better understand the situation I agree that changing the behavior of the |
Not saying we shouldn't change the behaviour, only that we need to decide on what that change should be. I did a quick search to find this pattern elsewhere and didn't find anything. Perhaps the solution, at least for now, is disable the resize button if it borders a collapsed panel? |
@chandlerprall I think that could be a good option. In particular, I think that when there are only two panels it wouldn't feel weird to the user. That's the use case in Discover. Things get trickier when there's more than two panels. I found a similar interface elsewhere. There, because the UI is a bit different (basically the "resizable button" is the panels’ borders) it’s more clear that it makes sense to disable resizing of a panel if it borders one that's been collapsed. In the gif below, if I could collapse HTML I wouldn’t be able to further resize JS or CSS and that would feel ok. JS only affects HTML (and viceversa) and CSS only affects HTML (and viceversa). With HTML collapsed, nothing should affect the JS panel's width. |
Works for me - let's go the route of disabling resizing any panel that neighbors a collapsed one. We'll get you engineering support on the panel registry changes. |
I'll most likely be the one helping here. A question that immediately comes to mind: |
Good catch, that's my interpretation as well. |
Awesome. I think ideally
|
Perfect |
Question on sizing for sibling panels after a panel is collapsed (with 3 or more panels): Given:
And B collapses, above examples show as below, where A and C grow equally:
But what about collapsing A or C instead? Do we expect all sibling panels to grow equally even if not a direct sibling?
or
If all siblings do not grow equally, does that change the assumption about A and C growing equally in the first example? Note that the equal growth idea might have been more a product of using CSS flex than anything; equal growth was the only real option. We have to do all the math ourselves now, so all options are on the table. The fact that this was all done with CSS before is really cool, @andreadelrio |
This ^ all gets even more tricky if we try equal panel growth, but not all panels were sized the same to start. |
I like to try to think these through in terms of real-world scenario/use-cases. For instance, I have a need to use these in Lens to be able to allow the user to resize the left and right sidebars as well as fully collapse them if they need the space. The sidebars have defined widths and the center takes up the remaining. When the user adjust the right side panel, it is resizing the right sidebar and the center is compensating. When the user collapses the right sidebar, the same thing happens. It adjusts the sizing of the right sidebar while the center grows to fill the space. Here is a video and a prototype of the collapsing behavior (drag to initiate toggle). Essentially when one panel adjusts only it's direct sibling adjusts as well. If it has 2 siblings, some maths to find the percentage of growth needed to add to each panel? |
Yeah, it's the 2+ sibling case that I was really asking about. I think adding to each sibling makes sense, just want to be sure |
Spent some time trying outlining a spec for behavior this morning, and I have some more questions 🙂 Based on all the discussion in this thread, it seems there are 2 ways to implement a toggle button, each having different impacts on expected behavior. Use the following for both:
First: Toggle button on the drag handle Given the example where B and C both have Directionality is therefore implied.
Second: Toggle button anywhere except the drag handle Given the example where B and C both have Directionality cannot be assumed like in the first example.
Regardless of above:
|
@thompsongl I personally don't have a strong preference for either implementation. I'd say go with the simplest one which it sounds like it's the first one?
I think definitely. Think of the Lens use case Caroline shared above, the sidebar should also be collapsible.
Ideally yes. |
Exactly. We could update the min-width on that example to be a bit more realistic, I bet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed everything except the reducer's logic, will do that one in a second pass.
- The Horizontal resizing with three panels example feels like it has been superseded by Resizable panel options
- Navigating to the collapse button via keyboard, there is no focus state on the button
src-docs/src/views/resizable_container/resizable_panel_collapsible_external.js
Outdated
Show resolved
Hide resolved
]; | ||
|
||
export default () => { | ||
const collapseFn = useRef(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be refactored to not rely on setting collapseFn as a side effect of the render? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing is immediately coming to mind for a refactor. The idea was to handle this similar to how ref
s are handled.
Co-authored-by: Chandler Prall <[email protected]>
Preview documentation changes for this PR: https://eui.elastic.co/pr_3978/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3978/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first example is no longer resizable by mouse 😕
Still working on the unresolved comments.
I agree; removed.
Added some styles. Feedback welcome.
Fixed. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3978/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3978/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3978/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM! The +2,675 −620 modified lines of code over 30 files does not fully represent the work y'all put into this feature. This is huge, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality all looks great. I'm sure we might find some more strange situations but, we won't know til we release 😄
Just had a few more suggestions for the docs.
src-docs/src/views/resizable_container/resizable_container_example.js
Outdated
Show resolved
Hide resolved
src-docs/src/views/resizable_container/resizable_container_example.js
Outdated
Show resolved
Hide resolved
src-docs/src/views/resizable_container/resizable_container_example.js
Outdated
Show resolved
Hide resolved
src-docs/src/views/resizable_container/resizable_container_example.js
Outdated
Show resolved
Hide resolved
src-docs/src/views/resizable_container/resizable_container_example.js
Outdated
Show resolved
Hide resolved
src-docs/src/views/resizable_container/resizable_container_example.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Caroline Horn <[email protected]>
Preview documentation changes for this PR: https://eui.elastic.co/pr_3978/ |
Summary
Adds the ability for
EuiResizablePanel
components to be collapsible—shrinking the panel itself and hiding its content either via an out-of-the-box toggle button or through coordinated actions hooks provided to the parent component.Features:
mode
prop specifies anEuiResizablePanel
ascollapsible
ormain
, indicating the ability to collapse or the preference to gain space from collapsed panels.EuiPanel
as the base element, allowing for more panel style optionsBreaking change:
Removed the
size
prop onEuiResizableButton
. This was originally added as something of a spacing mechanism, but is less effective than the newly addedpanelProp.paddingSize
prop.Checklist