[terra-menu] Dynamic resizing of sub-menu #992
Conversation
|
Dynamic resizing can be seen in this test example. Let me know your thoughts @jmsv6d @StephenEsser |
| @@ -146,6 +143,13 @@ class MenuContent extends React.Component { | |||
| } | |||
| } | |||
|
|
|||
| setPageHeight(node) { | |||
There was a problem hiding this comment.
You shouldn't need to do this calculation. Popup should handle this without setting an explicit height on the content. You can change this https://github.com/cerner/terra-framework/pull/992/files#diff-8821ddb6189a1accf05f7aef26338373R344 to be
const contentHeight = this.props.isHeightBounded && '100%' ;
And update the content container's fill prop to be just this.props.isHeightBounded
There was a problem hiding this comment.
updated here b912052
|
Is this acceptable behavior? @neilpfeiffer |
StephenEsser
changed the title
lokesh-0813
requested review from
dkasper-was-taken,
mjhenkes,
ryanthemanuel and
tbiethman
as code owners
lokesh-0813
requested review from
supreethmr,
avinashg1994,
nramamurth,
pranav300 and
saket2403
| <Menu.Item text="Default 3.0" key="3.0" className="TestNestedMenuContent" />, | ||
| <Menu.Item text="Default 3.1" key="3.1" className="TestNestedMenuContent" />, | ||
| <Menu.Item text="Default 3.2" key="3.2" className="TestNestedMenuContent" />, | ||
| <Menu.Item text="Default 3.1" key="3.3" className="TestNestedMenuContent" />, |
There was a problem hiding this comment.
Nit: It should be Default 3.3 instead of 3.1.
There was a problem hiding this comment.
Fixed here 9515f20
|
@lokesh-0813 Are there no wdio screenshots that need to be updated with this change? If not, then I'd recommend adding one. |
|
| @@ -3,6 +3,8 @@ Changelog | |||
|
|
|||
| Unreleased | |||
| ---------- | |||
| ### Changed | |||
| * Removed fixedHeight logic from Menu. | |||
There was a problem hiding this comment.
Can we update this descriptions o it's meaningful to users? Maybe something like we removed the fix height in sub-menus or we allow sub-menu's height to be determined by it's content?
|
@lokesh-0813 I am noticing different behavior of bound menus when the sub-menu is clicked. It seems the sub-menu is not longer bound. Here is a gif of the behavior seen in the deployment vs the existing behaviors found on https://engineering.cerner.com/terra-framework/tests/terra-menu/menu/menu/bounded-menu. |
@emilyrohrbough It's expected, since sub-menu has lesser menu-items compared to main-menu menu-items. Now sub-menu determines its height based on the number of menu-items it possess. I have tweaked the Bounded Menu example(removed main-menu items and added more sub-menu items) to give you an idea of what's going on -> Existing behaviour(master Branch) Deployment behaviour |
I thought a bounded menu's height was determined by i's parent container, not its contents. Is that not the case? |
| @@ -350,7 +346,7 @@ class MenuContent extends React.Component { | |||
| role="dialog" | |||
| onKeyDown={this.onKeyDown} | |||
| > | |||
| <ContentContainer header={header} fill={this.props.isHeightBounded || this.props.index > 0}> | |||
| <ContentContainer header={header} fill={this.props.isHeightBounded}> | |||
There was a problem hiding this comment.
why was the index prop used here before ?
There was a problem hiding this comment.
index > 0 would imply that it is sub-menu that is being rendered.
There was a problem hiding this comment.
so how we are handling this scenario now ? after removing this ?
cc : @neilpfeiffer |
|
@emilyrohrbough A menu/popup is "bounded" depending on if the height or width is greater than that of the bounding container. With the menu having a dynamic height, the state of if it's bounded or not can change. This is how I would expect this to work for this situation, I don't think there is an efficient way to get around it, especially for the reverse situation that @lokesh-0813 showed. But it's one of the visual oddities that we should have ux verify if it's acceptable, to determine if this is the actual approach we want to take to fix the original issue of if the submenu is much larger than the main menu, the submenu can end up with a very small scrollable area. |
|
Closing until we identify the appropriate solution on the parent issue: #952 |
fixes : #952