? ? Pending

User tests: Successful: Unsuccessful:

avatar vintzl
vintzl
27 Aug 2019

Pull Request for Issue #26022.

Summary of Changes

Remove all references of set(hidemainmenu) outside configuration.php. Now the behavior of the side menu is no more hard-coded.

Testing Instructions

Check if side panel appears where editor is used.

Expected result

Side panel can be enable (hidden by default).

Actual result

Side panel can be enable (hidden by default).

Documentation Changes Required

None?

avatar vintzl vintzl - open - 27 Aug 2019
avatar vintzl vintzl - change - 27 Aug 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 27 Aug 2019
Category Unit Tests Repository Administration com_admin SQL Postgresql MS SQL
avatar franz-wohlkoenig franz-wohlkoenig - change - 27 Aug 2019
Title
4.0 dev sidebar
[4.0] dev sidebar
avatar franz-wohlkoenig franz-wohlkoenig - edited - 27 Aug 2019
avatar C-Lodder
C-Lodder - comment - 27 Aug 2019

Why are there so many changes on this PR?
Looks like to need to fetch from upstream and merge into your branch...

avatar mbabker
mbabker - comment - 27 Aug 2019

“hidemainmenu” really was a bad name for the internal var, but that’s another story. In 3.x and earlier, when that is set it disables the menu links but keeps the menu visible. You’re changing the semantics of it here. IMO, it should keep the same semantics it always had (disable the menu links), then have another template level option to default show/hide the menu on edit views.

Remember, there is still the UX/workflow issue to sort out of not navigating out of the edit view using the toolbar items (close/cancel) leaving the item in a checked out state. Until that is addressed, I think it’s a bad idea to give the menu a visible state with all links active. So showing the menu is fine, but similar to now it should be inactive.

avatar vintzl
vintzl - comment - 27 Aug 2019

@C-Lodder Sorry, i screwed up with the PR :s

@mbabker

  1. The menu is disable by default
  2. I will check the name, etc…
  3. This is the first step, then I will try to fix the issue #4773 / #5815
avatar C-Lodder
C-Lodder - comment - 27 Aug 2019
$ git remote add upstream https://github.com/joomla/joomla-cms.git
$ git checkout 4.0-dev-Sidebar
$ git fetch upstream 4.0-dev
$ git merge upstream/4.0-dev
$ git push origin 4.0-dev-Sidebar

Think that will do the trick

avatar mbabker
mbabker - comment - 27 Aug 2019

63c5804 makes too many changes then. Components are responsible for signaling that the menu should be in a limited state, so removing the line from all the views should not happen.

What you really should do is add a template param (not a global config option, because the sidebar is specific to the Atum template) and check that param as well as the input variable. What you have now is a global config variable (which implies it affects everything in Joomla) and code that reads that variable on all pages. Or, IMO just get rid of the show/hide sidebar feature because you really aren’t gaining much with it in the first place.

avatar vintzl
vintzl - comment - 27 Aug 2019

63c5804 makes too many changes then. Components are responsible for signaling that the menu should be in a limited state, so removing the line from all the views should not happen.

I do not think so. Having any value hard-coded/set in so many views is a very bad practice (not very MVC compliant)… This is at least error prone, and should be done in one place, not in so many files as it is the case…

What you really should do is add a template param (not a global config option, because the sidebar is specific to the Atum template) and check that param as well as the input variable. What you have now is a global config variable (which implies it affects everything in Joomla) and code that reads that variable on all pages.

Good point.

Or, IMO just get rid of the show/hide sidebar feature because you really aren’t gaining much with it in the first place.

I disagree… I really think that a static left panel + something like #4773 / #5815 will be very simple, but good improvement.

avatar mbabker
mbabker - comment - 27 Aug 2019

The problem with your commit and your proposed change is you are changing the template behavior from basically only putting the menu in a limited functionality state on edit views by default to potentially putting it in a limited functionality state on all pages. That's not what you want here. You might view that line being repeated in 20 components as an error prone hardcoded behavior, but that is exactly how a modular MVC based system should function. You are removing control of a system behavior from the component, where it has existed for 14 years without an issue.

The component should still be responsible for signaling "the menu should be in a limited function state" until whatever comes out of the newest iteration of #4773 or #5815 is actually implemented. And once those are done, then honestly the line you've removed from all the component views can go away completely.

You want to be able to set the sidebar to always be expanded. That is a new template parameter and adjusting the template's index.php file to account for that parameter. That is not changing how an internal system variable behaves.

avatar vintzl
vintzl - comment - 27 Aug 2019

The problem with your commit and your proposed change is you are changing the template behavior from basically only putting the menu in a limited functionality state on edit views by default to potentially putting it in a limited functionality state on all pages. That's not what you want here.

  1. As you can see, this is a draft, and only a draft.
  2. I already wrote that I am far from being a Joomla expert, and I hope to get some feed back here to fix some bad decisions I made (and win some time)… And thank you, you do the job. Yeah, I know I am cheating but I am not even a PHP developer, but I repeat C#.

You might view that line being repeated in 20 components as an error prone hardcoded behavior, but that is exactly how a modular MVC based system should function. You are removing control of a system behavior from the component, where it has existed for 14 years without an issue.

These lines repeated in several component's view (JFactory::getApplication()->input->set('hidemainmenu', true); and JFactory::getApplication()->input->set('hidemainmenu', 1); and other variants) are definetly not MVC.

In MVC (in its pristine form, like Trygve Reenskaug), the view is so stupid that it can only:

  • renders text;
  • iterates through list;
  • certainly NOT modify behavior.

Even after 14 years, bad decisions/design can be fix, as well as my Joomla's nooby trying.

avatar mbabker
mbabker - comment - 27 Aug 2019

You're right that the view isn't the right place to be setting that request var and that it'd be better suited in a controller, but my point is it is a behavior that is triggered by the component and I don't think this should be taken out of the component's scope to decide UNTIL Joomla can be made to work so that leaving an edit screen by any means other than the toolbar buttons doesn't leave unwelcome side effects. This is why I'm saying you're better off right now with a parameter in the template that sets the sidebar state, or just flat out removing the logic to show/hide the sidebar by default and just have it always default to expanded open (and if someone is really adamant about having this functionality, then I'd honestly change it so it's 100% JavaScript driven and use localStorage to store the state of the sidebar).

With the change you've made, as is you are defaulting the menu to be in the limited functionality state for the entire backend interface. This is creating the side effect of there not being a sidebar at all anywhere.

Again, there are two separate actionable items to be worked on:

  • The inconsistency of not having the sidebar rendered on edit views but having it rendered in list/dashboard views
  • The UX issue of navigating away from an edit screen without using a toolbar action causing undesired side effects in the system (namely the item is left in a checked out state)

You're trying to tackle both in one go with an over-eager (and massively B/C breaking) change.

avatar vintzl
vintzl - comment - 27 Aug 2019

You're trying to tackle both in one go with an over-eager (and massively B/C breaking) change.

I do not think so. As I have written earlier, I want to

  • first work on this side panel and get what I want, and seems simbus82 too,
  • then fix the UX issue.

Maybe I am wrong, but seems that, at least, @brianteeman and @dgrammatiko would prefer to keep things unchanged (even if I do not understand the later; how a toggle menu would take 1/3 of our 16:9-16:10-21:9 screen space).

So what I intend to do is to propose something which will please everyone. Then work on the UX issue. But maybe I am wrong. I'll take a look to the UX issue in depth, and maybe I will start there.

avatar brianteeman
brianteeman - comment - 27 Aug 2019

Only speaking for myself but I believe you are confusing your idea of a permanent but disabled sidebar with a proposal to allow unrestricted use of the menu

avatar mbabker
mbabker - comment - 27 Aug 2019

I think we're talking past each other right now. Let me try to explain here.

The lines you removed from the components are the lines that signal to the system (mainly the admin menu module and the template) that the menu should be in a limited functionality state. In 3.x and earlier, this basically only disabled the links in the menu module while still rendering the menus; with 4.0 (and specifically the Atum template) this is also telling Joomla to not render the sidebar on a page at all.

The issue people are presenting is that the sidebar does not render at all on edit views. Opinions on that aside, if the desired state is to have the sidebar render on edit views, then you should back out any code that explicitly blocks rendering the sidebar without touching any other code, which looks to be removing the if conditional around this block of code.

Now, for the issues with your changes in 63c5804. You have removed the signal from components that the menu should be in a limited functionality state and have changed the admin menu module and Atum template to use a global configuration parameter, defaulting to true which means that by default the menu on all pages is in the limited functionality state, which means with Atum there is no sidebar rendered at all. In other words, your change proposal actually doesn't work in the way you seem to intend it to and it actually leaves the backend in a broken state because without a manual edit to configuration.php you can't get a sidebar enabled.

So starting from a clean 4.0-dev branch, these are the changes you need to make to deal strictly with the fact that the sidebar does not render at all on edit pages:

  • Option 1: remove the if conditional noted above
  • Option 2: add a parameter to the template to toggle whether the sidebar is displayed when the menu is in a limited functionality and change the if conditional noted above to check against that parameter as well as the input variable for putting the system in that state

That will address the "sidebar does not show at all" issue without intermixing any other issues into the fix.

avatar mbabker
mbabker - comment - 27 Aug 2019

(And for clarity, the UX related issues deal with navigating out of edit views without using the toolbar, specifically any close/cancel type button, which causes the item to remain in a checked out state, which is why the menus go into a disabled state in the first place to try and steer users away from just clicking any random thing on the page; until that workflow issue is addressed the menu should not be rendered in an enabled state on edit pages, but again that is a totally separate issue to deciding whether the sidebar should render on edit pages or not)

avatar vintzl
vintzl - comment - 27 Aug 2019

@mbabker Thanks for you explanation. I did not notice that the variable was responsible of displaying and hidding the menu in the entire Joomla scope, as the name is hidemainmenu.

What we would say in Java/C# world, is that this name is lying… This name sucks :s

A better name, for such a scope would have been something like disableSidePanel, shutdownSidePanel, or unloadSidePanel. Yet, this variable is responsible for two things, so it still weird…

I use to think that this variable was only intended to deal with editing, where there are forms. Not in all the website. Why should it be otherwise? For what purpose? It is illogical for such a variable to cover such a scope. I cannot see any valuable reason.

And for you and @brianteeman.
What I want is

  1. a permanent sidebar,
  2. but restricted when done "are pending". So in this context, this is not "a totally separate issue to deciding whether the sidebar should render on edit pages or not". As, at the same time I will fix the UX issue…
avatar mbabker
mbabker - comment - 27 Aug 2019

A better name, for such a scope would have been something like disableSidePanel, shutdownSidePanel, or unloadSidePanel. Yet, this variable is responsible for two things, so it still weird…

The input variable "hidemainmenu" has existed long before the Atum template and 4.0, it dates back to Joomla 1.5 where it hid the "submenu" module position, and continued like this up until 2.5 (sorry, I don't have a PHP platform old enough to load up a 1.5 or 2.5 site to see what the submenu position actually was, and nobody could pay me enough to painfully find a PHP 5.3 or 5.4 platform to actually do that). So changing the name of it at this point, confusing as it might be now, would be a B/C break without much gain (and no, I don't consider renaming things for the sake of renaming to be a strong reason for a B/C break myself).

I use to think that this variable was only intended to deal with editing, where there are forms. Not in all the website. Why should it be otherwise? For what purpose? It is illogical for such a variable to cover such a scope. I cannot see any valuable reason.

In core uses, it is only used on edit views to basically turn the menu module into a list of text items without any actionable links (since HTML lacks a proper disabled attribute for <a> elements). It is only with this design of the template that the input var signaling to disable the menu also turned into something that completely hides a section of the template. Theoretically, extensions could also trigger this behavior in their own contexts if they wanted to disable the main navigation in a component view for some reason (a component which has some kind of wizard would probably be a good example of a valid use of this in a non-edit context, Akeeba Backup also triggers this while a backup is running). I'm not tied to either way of handling things (completely hiding the thing or rendering the thing in a limited state), but I do think it would be best to keep treating that variable using the in-practice behavior it seems to have had through the 3.x lifetime, which was to disable the links in the menu module while continuing to render the module (which, in the current template, is the sidebar that is now hidden).

avatar vintzl
vintzl - comment - 28 Aug 2019

You know what, it's too much for me.

With all due respect, Joomla is obviously:

  • rigid as a single change causes a cascade changes in dependent modules.
  • fragile as it breaks in many places when a single change is made (here for a simple draft concerning a left panel). Hence, we are afraid to do some changes…, thus this "hidden hack" that is used to get around the UX issue…
  • MVC was clearly not understood by the one who wrote this spaghetti code (here involving (JFactory::getApplication()->input->set('hidemainmenu', true);), and leads to tidely coupled components, the opposite of pristine MVC which is about of separation of concern…
  • some variables are lying, and adds to the confusion (a bad name is misleading, this is clearly a big problem)…

Now I understand why I found some Joomla's people psychorigid. This rigid system is so difficult to change, that it leads to psychorigidity…

I give up, it will be too much involving and for a poor result.

Anyway, thanks for all your help, patience and advice.

avatar vintzl vintzl - close - 28 Aug 2019
avatar vintzl vintzl - change - 28 Aug 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-08-28 05:54:31
Closed_By vintzl
Labels Added: ? ?

Add a Comment

Login with GitHub to post a comment