Skip to content
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

BugFix: Make outside-left and outside-right block regions really responsive, resolves #266 #643

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

prasanna-lmsace
Copy link
Contributor

No description provided.

@prasanna-lmsace prasanna-lmsace marked this pull request as ready for review April 28, 2024 07:30
@abias abias force-pushed the main branch 3 times, most recently from 5696e9a to b9fa4ca Compare October 21, 2024 15:33
@abias abias force-pushed the outside-regions-fix branch from cc24267 to 23228c8 Compare November 21, 2024 10:49
@abias
Copy link
Member

abias commented Nov 21, 2024

Thank you @prasanna-lmsace for contributing this fix and thank you for your patience with our pending review.

I have just rebased and force-pushed your branch and will now review it.

Copy link
Member

@abias abias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @prasanna-lmsace ,

many thanks again for working on this PR and sorry again for my long latency to review it.

I have just pushed a "Review changes" commit which is basically just housekeeping and comments / documentation.

Reviewing your changes, I did not spot major flaws on Firefox/Mac and Chrome/Mac. The outside-left and outside-right block regions are displayed on the side or wrapped properly based on the particular screen width.

As a side note: I think that the media queries / breakpoing handling for block regions have become even more complicated than they were before, but I do not have a good proposal how to simplify that, so let's keep it for now.

Having said that, I would like to highlight some breakpoint glitches where I think the proportions of the block region and the content region is not well balanced anymore.

Glitch 1a:

Browser window width: 1200px
theme_boost_union | coursecontentmaxwidth = 950px
Outside Left Region = Enabled
Outside Right Region = Enabled
theme_boost_union | blockregionoutsideleftwidth = 350px
theme_boost_union | blockregionoutsiderightwidth = 250px
theme_boost_union | outsideregionsplacement = Near Window Edges

On this window size, the outside blocks still keep their configured width.
Based on the fact that both the left and the right region are used, the content area gets really condensed:

grafik

My gut feeling is that, if (and only if) both the left and the right region is enabled and used, the blocks should already be shrunken in width to give the content area more space.

Glitch 1b:

Browser window width: 1200px
theme_boost_union | outsideregionsplacement = Next to main content area
All other settings are the same as with glitch 1a.

With this setting, the content area is even more condensed:

grafik

My change request here is the same as with glitch 1a.

Glitch 2a:

Browser window width: 990px
theme_boost_union | coursecontentmaxwidth = 950px
Outside Left Region = Enabled
Outside Right Region = Enabled
theme_boost_union | blockregionoutsideleftwidth = 350px
theme_boost_union | blockregionoutsiderightwidth = 250px
theme_boost_union | outsideregionsplacement = Next to main content area

On this window size, the outside blocks are already wrapped above and below the content area which is fine.

I have just noticed that the block regions are not fully aligned vertically with the blocks in the main area:

grafik

Would you agree that the block regions could use the same horizontal margins as the content area?

Glitch 2b:

Browser window width: 990px
theme_boost_union | outsideregionsplacement = Near Window Edges
All other settings are the same as with glitch 2a.

With this setting, it is surprising that the content area gets smaller and does not use the available width anymore:

grafik

This is different to the content area width with the "Next to main content area" setting and different to the content area width if you do not have any outside blocks enabled. I think this is just a glitch in the CSS selectors and can hopefully be solved quite quickly.


I would appreciate if you could have a look at these glitches, solve them and update the PR?
Please do not squash the PR, just add a new commit so that I just have to review your latest changes.

In addition to that, I have posted a small question to the code and would appreciate if you could answer it.

Many thanks in advance!
Alex

scss/boost_union/post.scss Outdated Show resolved Hide resolved
@abias
Copy link
Member

abias commented Dec 6, 2024

Review

Following https://github.com/moodle-an-hochschulen/moodle-plugin-maintaining/wiki/Check-list-for-peer-reviewing-patches-and-pull-requests

Documentation

  • [Y] Commit Message understandable and issue referenced (via resolves keyword)
  • [Y] Patch author correct
  • [Y] CHANGES.md appended
  • [-] README.md appended
  • [-] Credits appended
  • [Y] Appropriate Comment quantity
  • [Y] Successful Moodle PHPDoc check

version.php

  • [Y] Checked if $plugin->version increment necessary (and increment done if necessary)
  • [Y] $plugin->release untouched

lib.php

  • [-] Only necessary functions

Languages

  • [-] Only english strings
  • [-] Necessary magic strings added (e.g. capabilities)

Automated tests

  • [-] New functionality covered
  • [Y] No failing steps or scenarios

Mustache templates

  • [-] Example context exists

CSS and styles.css

  • [Y] Bootstrap styles used if possible

Duplicated code

  • [-] Duplicated code is marked properly

Github action

  • [Y] All green

Commit history and scope

  • [N] Already single commit
  • [Y] Focus on single purpose
  • [Y] No surplus files

Additional aspects for Boost Union

  • [Y] No usage of $theme->settings
  • [-] Usage of isset() checks when processing plugin settings in renderer / output code
  • [-] Usage of admin_setting_configselect instead of admin_setting_configcheckbox
  • [-] Modified mustache templates properly marked and .upstream template in place

@prasanna-lmsace
Copy link
Contributor Author

prasanna-lmsace commented Jan 28, 2025

Glitch 2b:

Hi @abias

Thank you for your detailed review and feedback on the PR.

We have carefully considered the highlighted breakpoint glitches and implemented the following improvements to address them:

Adjusted Block Region Widths: The left and right block region widths have been reduced to a maximum of 200px in responsive view, allowing more space for the main content block and improving usability.

Enhanced Main Content Width (Glitch 2b): The main content block region is now set to full width on responsive views, overriding its "Course content max-width" setting to achieve a more balanced layout.

We hope these updates resolve the layout balance issues across breakpoints. Please let us know if further adjustments are required.

Many thanks again for your time and valuable feedback!

Many thanks!
Prasanna

@abias
Copy link
Member

abias commented Jan 31, 2025

Many thanks, @prasanna-lmsace , for pushing this further.

I think your fixes for glitch 1a, 1b and 2a are fine. Thank you very much!

Unfortunately, your fix for glitch 2b seems not to have the desired result. I do not see that the content is expanding to full window width with the given settings. Well, at least partly. When the page is loaded, the content is full width for a fraction of a second and then it collapses to a smaller with.
Please see this screen recording for details which I tookl while reloading the page multiple times:
https://github.com/user-attachments/assets/9f888582-36f8-4a20-84c6-4dbe539e83ff

In addition to that, the fix for glitch 2b seems to have an undesired side effect on really large screens where the maximum width is not respected anymore as well.
Please see these screeshots:
Before:
grafik
After:
grafik

Would you mind to have another look at glitch 2b


In addition to that, I have to add a third glitch which I just encountered:

Glitch 3:

Browser window width: 1610px
theme_boost_union | coursecontentmaxwidth = 950px
Outside Left Region = Enabled
Outside Right Region = Enabled
theme_boost_union | blockregionoutsideleftwidth = 250px
theme_boost_union | blockregionoutsiderightwidth = 250px
theme_boost_union | outsideregionsplacement = Next to main content area

On this rather large window size, the content area used to get enough space based on the coursecontentmaxwidth setting:
grafik

However, with this patch (and otherwise unchanged settings), the content area is compressed to a width with less than the coursecontentmaxwidth setting even though there would be enough space:
grafik

Is this something which can be avoided?

Thanks in advance,
Alex

@prasanna-lmsace
Copy link
Contributor Author

Hi @abias

Thank you for highlighting the issues and providing detailed feedback along with the screen recording.

We’ve implemented the following updates:

Resolved Glitch 2b: The content now consistently expands to full window width as expected and no longer collapses after the page load.

Addressed Large Screen Behavior: The maximum width is now correctly respected across all screen sizes.

Fixed Glitch 3: The content area dynamically adjusts to the coursecontentmaxwidth setting, ensuring proper spacing on larger window sizes.

Please let us know if any further adjustments are needed.

Many thanks again for your time and valuable feedback!

Thanks
Prasanna

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