I love admin_menu, it's the top 1 module I donwload after drupal

But one thing I love about the Drupal 7 toolbar is the feeling of being fixed.

I would like to have that feeling with admin_menu. A solid toolbar.. and never again blinking, adjusting the top margin dynamically in *every* single page request. It's not pretty to see, specially in a little slow Drupal sites. It's similar to the FOUT problem that online font libraries have.

Why not follow the same same approach of D7 toolbar and render the admin_menu during page_alter instead of dynamically via js?

I know that a lot of time and energy has been invested already in the JS approach and the module uses client-side ajax caching, and what I'm suggesting is not as optimal, but I think could be worth it.

Other benefits, apart of stopping the visual glitching:
* I can use admin_menu with JS disabled ! (during debugging)
* Performance is improved saving HTTP requests (when admin menu is not cached)
* Avoiding reflow/paint in browsers.
* Light ups the client side javascript

I would say that more than 1/3 of the issues of this module related to javascript/ajax problems.

Here is a joke-minimal patch to make this happen. It's only meant as an idea to work on, please don't "review" it... I can make a proper patch if there is future or support for this.

CommentFileSizeAuthor
#10 temporary-blinking-fix.patch1.69 KBgaborpeter
#8 admin_menu.js-cache-config.8.patch1.39 KBsun
#5 client-side-caching-option-1194528-4.patch2.09 KBcorbacho
#2 client-side-caching-option-1194528-2.patch3.08 KBcorbacho
page_alter_no_js.patch5.02 KBcorbacho

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Dont use JavaScript to load it » Allow to disable client-side caching and JavaScript injection

We can do this, and all you actually need to do is to add a checkbox to the settings page that uses the key 'admin_menu_cache_client', defaulting to 1.

corbacho’s picture

corbacho commented
Status: Active » Needs review
StatusFileSize
new client-side-caching-option-1194528-2.patch3.08 KB

Great sun, most of the work was done with that variable. I tested in FF,Chrome and IE 7-9 also I tested with and without Toolbar style module.

Please review this patch. It's the first time I use a git branch method to create a patch so maybe I did something wrong there.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

Need to think about the checkbox form element wording. Humans need to understand this option. Most of the current description should be in the title. But nevertheless, regular end-users probably still won't have a clue, so we likely still need a new description.

+++ b/admin_menu.module
@@ -202,10 +202,27 @@ function admin_menu_suppress($set = TRUE) {
+  $margin_top = variable_get('admin_menu_margin_top', 1);
+  // If admin_menu is in the page_top region, assume that js injection is disabled.
+  if ($margin_top && isset($vars['page']['page_top']['admin_menu'])) {
+    $vars['classes_array'][] = 'admin-menu';
+  }

Just check for the variable and if it's enabled, add the class. No need for the $margin_top variable and the other condition.

Actually, now that I think through this, the adjustment via JS was introduced in a time when we were not able to add classes/attributes to the BODY element. It's possible that we can entirely remove the adjustment via JS and only keep this _preprocess_html(). That's a separate issue though - for now, let's simply add the class if enabled.

+++ b/admin_menu.module
@@ -202,10 +202,27 @@ function admin_menu_suppress($set = TRUE) {
 function admin_menu_page_alter(&$page) {
-  $page['page_bottom']['admin_menu'] = array(
+  $region = variable_get('admin_menu_cache_client', 1) ? 'page_bottom' : 'page_top';
+  $page[$region]['admin_menu'] = array(

This can be reverted, the CSS cares for the positioning.

+++ b/admin_menu.module
@@ -238,7 +255,7 @@ function admin_menu_js() {
-  if (!variable_get('admin_menu_cache_client', TRUE)) {
+  if (!variable_get('admin_menu_cache_client', 1)) {

@@ -261,7 +278,7 @@ function admin_menu_cache_get($cid) {
-  if (variable_get('admin_menu_cache_client', TRUE)) {
+  if (variable_get('admin_menu_cache_client', 1)) {

These two can be reverted.

corbacho’s picture

corbacho commented
StatusFileSize
new client-side-caching-option-1194528-4.patch2.09 KB

Oops, sorry I was busy. Here is the patch with the modifications suggested.

I tried a rewording of the checkbox

I still don't like the dynamic re-position of the margin-top, as you said there is no need to do it anymore in the client-side. I will open a new issue when this gets submitted.

corbacho’s picture

corbacho commented
Status: Needs work » Needs review

.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +D7 stable release blocker
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Allow to disable client-side caching and JavaScript injection » Allow to configure client-side caching
Status: Needs review » Fixed
StatusFileSize
new admin_menu.js-cache-config.8.patch1.39 KB

Thanks for reporting, reviewing, and testing! Committed attached patch to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Automatically closed -- issue fixed for 2 weeks with no activity.

gaborpeter’s picture

gaborpeter commented
Issue summary: View changes
StatusFileSize
new temporary-blinking-fix.patch1.69 KB

Situation: our client is complaining about the blinking :)
So we had a look as well, it is about the fact that JavaScript is applying the class name on the BODY tag.
We've seen also the comment in the admin_menu_page_build function that the class is not added becasue the function doesn't support it.
Also looks like hook_page_build won't be used in Drupal 8, so I was wondering to implement an another solution.

Here is a patch which implement the preprocess_html hook (the same way like views or panels does) and adds the class.
What are your opinions about this approach?