MudPopover: MudList MaxHeight Adjustments#11734
MudPopover: MudList MaxHeight Adjustments#11734danielchalmers merged 4 commits intoMudBlazor:devfrom
Conversation
Updated `mudPopover.js` to improve handling of popover maximum height, including checks for "mud-list" elements and adjustments based on viewport space. Ensured proper display in relation to the app bar and added logic for resetting max height for recalculations. Introduced a new test layout in `PopoverListMaxHeightTest.razor` to demonstrate the popover's functionality, featuring a `MudAppBar` with a menu. This test is linked to a related GitHub issue for tracking purposes.
github-actions
bot
added
the
bug
There was a problem hiding this comment.
Pull Request Overview
This PR fixes bugs in the MudPopover JavaScript logic that handles max height adjustments for MudList components (used in MudSelect, MudMenu, MudAutocomplete) when they would exceed available screen space after flipping.
- Refactored the height adjustment logic to be clearer and more maintainable
- Fixed three specific bugs: improper max height reset, unnecessary reset outside flip logic, and edge case where popover at exact top position incorrectly triggered max height setting
- Added a test component to verify the fix for issue #11730
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/MudBlazor/TScripts/mudPopover.js | Restructured popover height adjustment logic, fixed bugs in max height calculations and reset behavior |
| src/MudBlazor.UnitTests.Viewer/TestComponents/Popover/PopoverListMaxHeightTest.razor | Added test component demonstrating MudMenu behavior in app bar layout |
|
Could you take a look at the Quality Gate |
|
should have fixed all the sonarcube problems, however now it's mad because eslint is using a really old version. So if we can support ES2020 then update .eslintrc here: {
"parserOptions": {
"ecmaVersion": 2020
}
}or change line 323 to read (sonar will complain): const isList =
firstChild &&
firstChild.classList &&
firstChild.classList.contains("mud-list"); |
|
@gemini-code-assist review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the popover positioning logic, especially for lists that need to have their height adjusted to fit on the screen. The changes aim to fix several bugs related to max-height calculation when a popover flips.
Overall, the refactoring in mudPopover.js significantly improves the readability and structure of the height adjustment logic. The introduction of helper variables and functions like isUnset and checkHeight, and the clear separation of logic for upward-flipped and downward-opening popovers, make the code much easier to understand and maintain.
I've found one high-severity logical issue in the new JavaScript implementation that should be addressed, as it prevents the popover from using all available space in certain scenarios. Additionally, I've pointed out a minor typo in the new test component.
Once the identified issue is resolved, this pull request will be a great improvement to the popover's behavior and stability.
| availableHeight = anchorY - overflowPadding - popoverNode.offsetHeight; | ||
| shouldClamp = availableHeight < popoverContentNode.offsetHeight; | ||
| if (shouldClamp) { | ||
| top = overflowPadding; | ||
| offsetY = 0; | ||
| } |
There was a problem hiding this comment.
There appears to be a logic issue in the calculation of availableHeight when a popover is flipped upwards and needs to be clamped.
When shouldClamp is true for an upward-flipped popover, its top position is correctly reset to overflowPadding, moving it to the top of the viewport. However, availableHeight is not recalculated to account for this new position. It's still based on the original anchorY, which limits the popover's max-height to the space above the anchor, not the full available viewport height.
To fix this, availableHeight should be recalculated after the popover is moved to the top. This will allow the popover to use all the available vertical space as intended.
if (shouldClamp) {
top = overflowPadding;
offsetY = 0;
availableHeight = window.innerHeight - top - overflowPadding;
}There was a problem hiding this comment.
since shouldClamp is determined by the available height being less than the popover height, not defining available height first would not have the method know if it needs to be clamped. (e.g. would it exceed the screen before the anchor.)
src/MudBlazor.UnitTests.Viewer/TestComponents/Popover/PopoverListMaxHeightTest.razor
Outdated
Show resolved
Hide resolved
|
Resolves #11730
There is a section of code that triggers when a popover has
FlipAlwaysorFlipOnOpenthat is supposed to determine ifThen if it would exceed the available space it would assign a max height to both the popover and the mud list that would force a recalculation and repositioning resulting in a best fit scenario.
Found and Fixed 3 bugs in the calculation and reworked it to be easier to follow.
Bug 1) I was not resetting the list maxheight if I altered it during a reflow.
Bug 2) I was doing the reset outside of flip logic. Unnecessary
Bug 3) [and the one related to attached issue] If a popover started at exactly the the top position that matches the overflow it tried to set maxheight. Previously it was getting overwritten in a reflow but a previous fix brought this edge case bug out.
Checklist:
dev).