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

[J4] Removes fixed max-height on sidebar-nav on wide screen #33108

Closed
wants to merge 1 commit into from
Closed

[J4] Removes fixed max-height on sidebar-nav on wide screen #33108

wants to merge 1 commit into from

Conversation

regularlabs
Copy link
Contributor

In PR #31092 a fixed height was introduced to the sidebar-nav to fix things on mobile screens (smaller than md).
But this causes a useless scrollbar and height limit on the sidebar nav on wider screens.

So this PR removes it.

Check the styling of the sidebar in the global configuration before and after this PR.

Before:
image

After:
image

In PR #31092 a fixed height was introduced to the sidebar-nav to fix things on mobile screens (smaller than md).
But this causes a useless scrollbar and height limit on the sidebar nav on wider screens.

So this PR removes it.

Check the styling of the sidebar in the global configuration before and after this PR.
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Apr 11, 2021
@regularlabs
Copy link
Contributor Author

Also fixes this issue: #31928

@brianteeman
Copy link
Contributor

(it wasn't just for mobile screens @ceford do you want to explain what the problem was you were solving)

@ceford
Copy link
Contributor

ceford commented Apr 12, 2021

31092 had nothing to do with mobile screens or screen width in general. It was about height. The list of Help screens in particular is very long - you had to scroll down a long way for the later ones, and when clicked on nothing appeared to happen because the help screen is loaded into a frame out of site at the top. Frankly, the Options list works better this way too. So I object to this PR. It is a step backwards and does not seem to solve any particular problem. The scrollbar is not useless! The lack of indication that there are more items further down is a more general problem that needs its own solution.

@regularlabs
Copy link
Contributor Author

Well, I think it is very weird to have the sidenav as an extra scrollable section. It would only make sense if that part was fixed to the same height of the browser window. But now, on many screen sizes, you have to scroll the main window down to see the bottom of that nav section... and then scroll that section down even more.

In my opinion the 'fix' applied in #31092 is a crude way to solve something that causes more issues and frustration than it is worth.

Anyway... my 2 cents. Do with it what you want...

@regularlabs
Copy link
Contributor Author

PRs that cause discussions are not worth my time. So closing this.

@regularlabs regularlabs deleted the patch-4 branch May 13, 2021 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants