dev/core#1757 hide menu toggle when D7 toolbar is absent

[colemanw]

Overview

This is a reviewer's cut of #17359:

This PR hides the "adjust menu position" for non admin users. The reason for this is that the user would see an empty gap if they can't normally see the admin menu.

Before

After

Technical Details

We have updated the CRM/Utils/System/Drupal.php file so it includes a js/crm.drupal7.js script only for Drupal 7 installations since this issue is related to the Drupal menu. We take into consideration if the js/crm.menubar.js has already been appended for two reasons: 1 - we only want to hide this button when the CRM menu is present, and 2 - we insert the script after the CRM Menubar script because we need to use the toggle function inside there.

for the js/crm.drupal7.js file we need to wait for both the CiviCRM Menu and Admin Menu to be available to determine if we need to hide the toggle button and to hide button itself since it's included inside of the CiviCRM Menu. We know the CiviCRM menu is ready after listening for $(document).on('crmLoad', '#civicrm-menu') and the Admin menu is ready when the document is loaded on $(document).ready. Something important to consider for this implementation is that the CiviCRM Menu might be ready before, or after the document is ready. This is because the CiviCRM Menu is cached after it first loads. This is the reason why we need to listen for both events.

After we wait for both menus to be ready we check if the Admin Menu has been appended to the DOM, and if not, we hide the toggle button since there's nothing to toggle. We also set the menu in the right position in case this button was already toggled so we avoid showing an empty gap to the user. This can happen after toggling the button or when switching from an admin account to a normal user account.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[reneolivo]

Better use

CRM.menubar.initializePosition = CRM._.noop;

If we check the reference for CRM.menubar.initializePosition and find it's assigned to a random empty function, we might be scratching our heads as to what happened, but _.noop means that this was done on purpose.

Even better would be to lock this functionality:

CRM.menubar.lockMenuPosition();

If the menubar is locked, initializePosition() should throw a warning. There should also be a isMenuPositionLocked() function so we can safely run initializePosition() without triggering a warning. This is a more complicated approach, but it explains better what happened, why we should not use this function.

When Triggering a function, if nothing happens might lead to confusion so it's best to be explicit about it through a warning message (either a throw or a console.error, etc.). Also, another file should not override the original CRM.menubar.

Just leaving this as a comment, not an actual action item.

[colemanw]

@reneolivo thanks those are excellent comments and suggestons. It's inspired me to rework my changes to move all the logic into the CRM.menubar object. The upshot is that there's now a way to remove the toggle from JS and from PHP:

JS:

CRM.menubar.removeToggleButton()

PHP:

\Civi::resources()->addSetting('menubar', ['toggleButton' => FALSE]);

[colemanw]

@reneolivo can you confirm that you've tried it and this PR works for you?

[reneolivo]

@colemanw here are the results:

As an admin user:

✅ when logging-in for the first time, the toggle button is visible. (no cached menu)
✅ when navigating to a new screen after logging-in, the toggle button is still visible. (cached menu)
✅ Toggling the menu and-logging out, the menu position is remembered after logging-in again.

As a normal user:

✅ when logging-in for the first time, the toggle button is not visible. (no cached menu)
✅ when navigating to a new screen after logging-in, the toggle button is not visible. (cached menu)
✅ logging in as an admin, toggling the CRM menu so is below the admin menu, logging-out, then when we log-in as a normal user the menu is reset to the top position.

Regarding this comment:

The one piece missing was to prevent the toggle from re-appearing when the menu is periodically re-rendered (this happens sometimes, esp when 3rd party extensions add/remove items).

As a normal user, when we manually trigger CRM.menubar.initializePosition() the button is not re-attached to the menu, but if we manually trigger CRM.menubar.togglePosition() the menu position still changes.

For us this is not a problem since we don't have any extensions that manually trigger initializePosition or togglePosition. Just letting you know if this is an issue for you.