Skip to content

Conversation

@prasanna-lmsace
Copy link
Contributor

@prasanna-lmsace prasanna-lmsace commented Mar 27, 2025

Added an admin setting under the Blocks section, "Outside regions vertical alignment," to align the outside left and right regions vertically with the page content. This includes empty placeholder divs inside these regions and adjusts the height based on the page content offset to ensure proper content positioning.

@prasanna-lmsace prasanna-lmsace marked this pull request as ready for review April 9, 2025 04:25
@abias abias force-pushed the region-outside-vertical-offset branch from de03819 to 32bc175 Compare November 4, 2025 13:40
@abias
Copy link
Member

abias commented Nov 4, 2025

Hi @prasanna-lmsace ,

many thanks for working on this contribution and sorry that it took us so long to review this PR.

I just rebased it onto latest main and had a look at the code.
I think your solution approach is perfectly fine. I just added a "Review changes" commit with language and comment and settings refinements.

Then, when testing manually, I encountered a glitch:
As #643 has been merged in the meantime, I saw that the code adds too much space above the outside-left and outside-right regions on smaller viewports.

On larger viewports, it looked like this (which is perfect):
grafik

And on smaller viewports it looked like this:
grafik

My gut feeling was that the JS code should be changed in a way that the vertical alignment is only processed on larger viewports. On smaller viewports, it should not do anything. And when resizing from a larger to a smaller viewport, it should revert the vertical alignment and let Bootstrap do its job.

I just went ahead and added this detail to make the PR mergeable.
I will merge it as soon as the tests are green.

Cheers,
Alex

@abias abias force-pushed the region-outside-vertical-offset branch from ec44350 to b27981a Compare November 6, 2025 11:34
@abias abias force-pushed the region-outside-vertical-offset branch from b27981a to ab69a63 Compare November 6, 2025 11:34
@abias abias merged commit 72c257d into moodle-an-hochschulen:main Nov 6, 2025
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants