MudBlazor / MudBlazor

Blazor Component Library based on Material design with an emphasis on ease of use. Mainly written in C# with Javascript kept to a bare minimum it empowers .NET developers to easily debug it if needed.
http://mudblazor.com
MIT License
7.21k stars 1.18k forks source link

MudDialog: When content exceeds hight scroll only content, not title and action buttons #8785

Closed epithet closed 1 week ago

epithet commented 3 weeks ago

Currently, when the content height exceeds the available height in a dialog, the whole dialog becomes scrollable, which means that the title and the action buttons can be scrolled out of sight. In the "Scrollable Dialog" example from the documentation, two scrollbars are rendered next to each other unnecessarily when the browser window becomes too small: dialog-scrollable-before

This PR makes the content area scrollable, which ensures that title and action buttons remain visible as long as possible.

dialog-scrollable-after

Description

This simplifies both the implementation and the "Scrollable Dialog" example (even though the previous version of the example still works). The layout now essentially works like before the introduction of the focus trap, since the additional divs are ignored by the browser's layouting due to display: contents.

The top and bottom paddings of the content area have been made margins instead, so that the overflowing content is cut off at the right position when the content becomes scrollable.

The issues recently fixed by #8743 remain fixed.

How Has This Been Tested?

This has been tested with the MudBlazor.Docs.Server project and the following examples:

Type of Changes

Checklist

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.09%. Comparing base (28bc599) to head (e511acb). Report is 153 commits behind head on dev.

:exclamation: Current head e511acb differs from pull request most recent head 2a527aa. Consider uploading reports for the commit 2a527aa to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #8785 +/- ## ========================================== + Coverage 89.82% 90.09% +0.26% ========================================== Files 412 420 +8 Lines 11878 12213 +335 Branches 2364 2409 +45 ========================================== + Hits 10670 11003 +333 + Misses 681 666 -15 - Partials 527 544 +17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

epithet commented 3 weeks ago

Smart! Could you apply display: contents in the CSS files instead of in razor?

@danielchalmers sure, do you want it in _dialog.scss as a solution specific to the dialog, or should I introduce a new scss file for the focus trap, so that the focus trap never disturbs the layout?

danielchalmers commented 3 weeks ago

@epithet I think it makes a lot of sense to apply it to all focus traps. In fact, do you think it should apply to the _root div as well?

Could you confirm this all works on the MudMessageBox as is uses Dialog and has its own FocusTrap?

Thanks

danielchalmers commented 3 weeks ago

@epithet Do you think display: contents could fix #1167 by adding it to the MudTooltip?

epithet commented 3 weeks ago

@epithet I think it makes a lot of sense to apply it to all focus traps. In fact, do you think it should apply to the _root div as well?

@danielchalmers Yes, it has to be applied to both _root and mud-focus-trap-child-container. I originally applied it to _root via inline style from MudDialog.razor. Now, I apply it to both from CSS.

BTW, if someone really needs a different behavior, it can be overridden inline <MudFocusTrap Style="display: ..."> or with a more specific CSS selector such as .mycomponent > .mud-focus-trap { display: ... }.

Could you confirm this all works on the MudMessageBox as is uses Dialog and has its own FocusTrap?

I can confirm and I updated the PR description accordingly.

epithet commented 3 weeks ago

@epithet Do you think display: contents could fix #1167 by adding it to the MudTooltip?

Possibly. I added a comment to the PR.

epithet commented 3 weeks ago

@danielchalmers The vertical padding was different when DisableSidePadding="true". While converting the vertical paddings to margins, I only refactored and thus kept the different spacings. But it seems to me that the property should not have any influence on the top/bottom paddings/margins. Can you confirm this is intentional?

danielchalmers commented 2 weeks ago

The vertical padding was different when DisableSidePadding="true". While converting the vertical paddings to margins, I only refactored and thus kept the different spacings. But it seems to me that the property should not have any influence on the top/bottom paddings/margins. Can you confirm this is intentional?

I asked and didn't get a response so I'd say just leave it as it now even if it's weird.

epithet commented 1 week ago

@danielchalmers Is there something else I can do?

danielchalmers commented 1 week ago

@epithet Looks good, I think there are conflicts though

henon commented 1 week ago

Conflicts need resolving. Is this breaking or not?

epithet commented 1 week ago

@danielchalmers @henon conflicts resolved

Is this breaking or not?

No, there are no API changes.

henon commented 1 week ago

Thanks you!