Merge Administration Menu Dropdown module

guardian - February 24, 2008 - 09:01
Project:Administration menu
Version:6.x-3.x-dev
Component:Code
Category:feature request
Priority:normal
Assigned:Unassigned
Status:needs work
Description

hello,

i would love to see the features of admin_menu_dropdown integrated into admin_menu :)
that would prevent breakage when one of the two modules is updated (like it's the cae with admin_menu-5.x-2.3 and admin_menu-dropdown-5.x-1.4)

cheers

#1

Shawn Conn - February 25, 2008 - 18:15

The 1.5 release fixes the incompatibility issue. As far as merging goes, I wouldn't be opposed to it; it would make my tasks easier :) You would have to discuss it with maintainers of the Admin Menu module. One issue I think would come up is compatibility; position:fixed would break compatibility IE 6 (though I'm not sure if Admin Menu officially supports IE 6- I however, don't). There's also some issues with FireFox 2 (see issue queue), however I'm inclined to believe they are issues with FF2's rendering of CSS being that they seem to be non-existent in FF 3 beta 3.

#2

guardian - February 25, 2008 - 16:30

did you already try to contect the maintainers of admin_menu ? :)
i'm sure they would ask for a patch to their module but i guess it's worth it
cheers

#3

Shawn Conn - February 25, 2008 - 18:23

I have not talked to them about a merge. Right now, my only concerns with maintenance of Admin Menu Dropdown is that it is working with the latest 5.x release (and perhaps a 6.x branch when that happens). If you want such a merge to happen, you will have to take initiative on it. I do not know the Admin Menu maintainers plans & goals so the issues I've mentioned above could be a showstopper for Admin Menu.

#4

sun - April 4, 2008 - 13:06
Version:5.x-1.4» 5.x-1.x-dev
Category:feature request» task

A merge would be possible if AMD would implement conditional checks/workarounds for the already mentioned bugs in some browsers/platforms. For example, there are plenty of (JS-based) workarounds for position: fixed xbrowser compatibility, and I could even imagine that there is already a jQuery (slider?) plugin for this purpose.

On another thought, I would really like to see this module (AMD) evolve in terms of enhancing DAM's usability. It seems like there are already some users that really like the shortcut keys. For example, I've moved this issue into AMD's queue, which reduces DAM to the icon in the upper-left corner instead of hiding it completely. This specific idea could be enhanced so that the menu slides in on hovering the icon. Such type of things should happen in AMD. Be experimental, allow all kind of different usability enhancements. Whenever needed, I'm totally open to implement some hooks in DAM to allow even crazier things in AMD, or even just to make AMD's PHP/JS code smaller. Just let me know via DAM's issue queue.

#5

Deciphered - June 16, 2008 - 00:12

Hi Sun,

Can you provide any more information on the position: fixed issue? Like a way of reproducing the issue or one of the workarounds? I've looked into a few css alternatives and a few js alternatives but nothing comes close to achieving the goal as well as the standard position: fixed.

#6

dbabbage - December 8, 2008 - 13:44
Version:5.x-1.x-dev» 6.x-2.x-dev

Speaking as an outsider—but one who really (really!) likes both modules—it does seem to me this is a sensible suggestion... Admin Menu Dropdown is providing functionality that could sensibly be embedded within Admin Menu, rather than offering something entirely separate. I'd have thought it'd be useful to expose all users of Admin Menu to those options.

If this is going to happen, it'd make sense at this point to do this against 6.x-2.x-dev so I have updated the version on this issue.

#7

Deciphered - December 9, 2008 - 21:44

Hi dbabbage,

I too agree that this would be sensible, and would love to do it, however it's not my decision, it's up to Sun. Last time I mentioned it to him he seemed interested, however there are no firm plans to do anything at the moment.

#8

sun - December 9, 2008 - 22:23

I am still open to discuss a module merger further.

Please note that position: fixed can be enabled via the regular settings page of admin menu now. (see #310423: Add optional position fixed configuration setting)

Also, #331982: Add support for keyboard navigation adds a nice keyboard navigation feature to admin menu. Just showing/hiding the menu never convinced me personally, but I love the idea of navigating through the menu using the keyboard. So maybe you could team up with the other developer of that issue, squeeze your menu toggle shortcuts somewhere in the patch and we're one step closer? ;)

That said, are there any other features in this module?

#9

Deciphered - December 9, 2008 - 23:06

@Sun

I did notice the position: fixed change; in my opinion that was one of the most important features of Admin Menu Dropdown.
As for the keyboard navigation, I guess the reason I hadn't bothered looking into that thus far is that it seems to be completely outside of the scope of what Admin Menu Dropdown is, which is simply a toggleable visibility addon to Admin Menu.

I would be more than happy to start looking into writing a patch to add some Javascript extras (Admin Menu Dropdown + Keyboard Navigation) directly into Admin Menu to negate the need of a secondary module, but if you can layout some constraints of how you would like it done (ie, hooks for user addons, written as a sub-module, etc) it would be extremely helpful.

#10

sun - December 10, 2008 - 01:46
Title:merge with admin_menu ?» Merge Administration Menu Dropdown module
Project:Administration Menu Dropdown» Administration menu
Version:6.x-2.x-dev» 6.x-1.x-dev
Category:task» feature request
Status:active» needs review

That's an interesting question, yes. Currently, more and more JavaScript-based features are added to admin menu, #220100: Add hoverintent support probably being the next one.

The fear I have is that not everyone wants to use those features. While hoverintent may be an exception to this, I think that further features should go into new JavaScript files that ship with admin menu, but are only loaded when the corresponding feature has been enabled in the admin menu's settings.

However, those additional features should follow a certain structure, and most importantly, they probably need to be executed in a certain stage. So I'm thinking about JavaScript "hooks", which allow add-on scripts and other modules to process the menu in certain stages. Note: These are completely wild and "unleashed" thoughts to have a starting point:

Similar to Drupal.behaviors, we define one or more stacks of functions to execute: Drupal.adminMenu.behaviors. Each registered function is executed once the DOM is loaded.

Hm. Turned out to be easier than I thought.

Attached patch should implement the core functions that will allow us to enhance administration menu's features via separate JavaScript files. As of now, there is only one @todo in the patch, which needs to be solved.

Also, moving this issue into admin menu's queue to (hopefully) gather more attention.

AttachmentSize
admin_menu-DRUPAL-6--1.behaviors.patch 3.75 KB

#11

Deciphered - December 10, 2008 - 02:44

I actually started working on an option, which is a simple hook system similar to what I started working on in admin_menu_dropdown. The main difference is it would allow for two types of 'tweaks', visibility tweaks and stand-alone tweaks.

However, I probably should have waited so that this exact issue wouldn't occur, but I hoped that maybe you where situated in a timezone that was in sleep mode so that I could get some stuff done.

I'll put my stuff on hold for the minute and have a look at your patch and see if I can figure it out.

Edit: Ok, doesn't look like this should effect any of my work, will continue and post a patch once I've got something usable.

#12

Deciphered - December 16, 2008 - 00:08

@sun

This patch incorporates your previous patch, and adds some minor hooks that allow for addon 'tweaks'.
So far there are two tweaks implemented, stanislav.mueller Keyboard navigation (a stand-alone tweak) and Admin Menu Dropdowns original key combo behavior (a visibility tweak).

The patch isn't 100% complete, I still want to cleanup the settings page and implement the Icon Hotspot visbility tweak, but I wanted to make sure I was going down an acceptable path.

Let me know what your opinion is and I will continue from there.

#13

Deciphered - December 16, 2008 - 00:09

Patch attached.

AttachmentSize
admin_menu-6.x-1.x-dev-226012-admin_menu_dropdown-1.patch 21.55 KB

#14

Deciphered - December 16, 2008 - 01:24

Updated the patch with the Icon Hotspot visibility tweak and a cleaner settings page.

AttachmentSize
admin_menu-6.x-1.x-dev-226012-admin_menu_dropdown-2.patch 23.15 KB

#15

Deciphered - December 16, 2008 - 02:31

And one more for luck: Improved usability of visibility tweak settings form.

AttachmentSize
admin_menu-6.x-1.x-dev-226012-admin_menu_dropdown-3.patch 24.33 KB

#16

sun - December 16, 2008 - 17:49
Status:needs review» needs work

Uh oh. I did not review the entire patch, but: Eval is evil. Avoid using eval() at all costs.

However, the primary reason why I did not review it completely was that I did not really understand the purpose and what you are doing in this patch. Also, what is the difference between "standalone" and "visibility" tweaks? And why do we need JS(ON) callbacks for them? Are those executed on every page request? If they are, this needs to be rewritten.

If you can, please provide a more detailed description of your patch, similar to the one in #10.

Also, for simplicity, please remove the changes for keyboard navigation from this patch. Those should be dealt with in #331982: Add support for keyboard navigation only.

#17

Deciphered - December 16, 2008 - 20:33

@Sun

I will have a look at an alternative to using eval, but it is there to call the user defined javascript function. I might be able to do something similar to the way you did with your JS changes.

The reason I defined two different type of tweaks is that every visibility tweak is essentially doing the same thing but in a different manner, therefore you never want to visibility tweaks operating at the same time. Visibility tweaks also have access to some predefined JS API functions that provide some of the more important functions, primarily setting the visibility status as a $_SESSION variable via the said JSON callback.
Standalone tweaks can do whatever they wish and therefore should not be restricted to only one running at anytime.

As said above, the JSON callback is to toggle the $_SESSION variable and as such is only called initially to fetch the visibility status, and then anytime the visibility status is toggled. The user who requested this feature initially suggested cookies to achieve the end but I felt a $_SESSION variable was a better fit. The point of this functionality is so that if a user toggles the state to hidden it will be hidden on the next page they load, and vice versa.
If you have a valid alternative to this I will be happy to re-write it.

As for the Keyboard navigation, I will remove that ASAP. The reasons I put it in was as suggested by you above, to show how it would integrate with my changes/hooks and to show an example of how a standalone tweak would be implemented.

Will re-write the patch within the next hour or so.

Cheers,
Deciphered.

#18

sun - December 16, 2008 - 20:52

Is it possible that you mean "admin_menu related tweaks" with "visibility tweaks", and "unrelated/common administration tweaks" with "standalone tweaks"? Sorry if I was not clear in my last reply - I actually do not understand the entire approach/concept and flow behind your patch. This does not mean it's bad or wrong, it's just that I would appreciate some "theory" we can discuss in parallel to code. And, I'm asking on purpose, especially because of #345984: Client-side caching of administration menu, which will need similar settings stored per user and/or session.

#19

Deciphered - December 16, 2008 - 21:40

@sun

"visibility tweak" - A tweak/add-on designed only with the intent to toggle on/off the visibility of the Administration Menu.
"standalone tweak" - A tweak/add-on designed with any other purpose than the above.

While these may not be the best categories for tweak types, given that I am coming from AMD it is where my mind is at this point.

As for theory, the way I see it is as such:

- As I said earlier, visibility tweaks, tweaks that only deal with the visibility of the Administration Menu, can not run in parallel, therefore they need a definition so that a select box of choices can be provided.

- Once a visibility tweak has been loaded it will be activated via 'Drupal.adminMenu.visibility', a javascript function that works with your new modular system. The tweak itself can not be a direct part of your modular system as it wouldn't allow for multiple visibility tweaks to be defined in one .js file.

- 'Drupal.adminMenu.visibility' will run the 'init()' function of the defined visibility tweak via eval() to run it's initial behavior (binding events, etc), and then checks the default/set status of the Admin Menu visibility and sets it as such.

- The defined visibility tweaks must define the events to show and hide the menu, and point them to the helper functions of 'Drupal.adminMenu_visibility', which will set the visibility status in the $_SESSION data, set the 'Drupal.settings.admin_menu.tweak_visibility.hidden' variable and then pass back to the visibility tweak to actually process the hide/show function as defined.

- Standalone tweaks have the ability to set settings on the Admin Menu settings form, but this is entirely optional as they are completely standalone and realistically could be defined without using any of Admin Menu's hooks.

I hope I'm shining some light on my reasoning and not just confusing the matter more.

#20

Deciphered - December 16, 2008 - 23:17

Updated patch with the following changes:
- Keyboard navigation standalone tweak has been removed.
- Visibility tweaks no longer initiated via eval() call.
- Visibility tweaks now have to be defined as 'Drupal.adminMenu_visibility.tweak.name'.
- Improved documentation

There is still an eval() call in the 'Key combo' visibility tweak, but that is a necessary evil so that users can define a custom combo.
The JSON callback is unchanged as I don't see a cookie achieving the same end and can not see an other viable alternatives.

Look forward to your feedback.

AttachmentSize
admin_menu-6.x-1.x-dev-226012-admin_menu_dropdown-4.patch 17.73 KB

#21

dbabbage - December 17, 2008 - 02:24

.tweak seems like a rather opaque namespace... Given adminMenu_visibility, and a unique name appended to this, is "tweak" actually necessary?

#22

Deciphered - December 17, 2008 - 02:44
Status:needs work» needs review

Updated patch, fixing a bug from bad testing practices and improving the namespace.

@dbabbage
I had just written a fix for a bug related to the namespace and was in the process of submitting it.
While writing the comment I was also in the process of answer your question which an admission of my dislike for it myself and a justification of it's necessity when it twigged that not only was it not necessary, it was also introducing more complexity and issues than I had thought it was solving, so thank you :)

AttachmentSize
admin_menu-6.x-1.x-dev-226012-admin_menu_dropdown-5.patch 17.68 KB

#23

sun - December 17, 2008 - 09:33
Status:needs review» needs work

Instead of .inline-element, you probably want to use Drupal core's default class .container-inline.

Now I'm a bit stuck - how many visibility tweaks are there? Am I blind or is there just one?
Depending on the answer, we probably want to shift the "sub-behaviors" in Drupal.adminMenu into Drupal.adminMenu.behaviors and thereby make room for a new namespace in Drupal.adminMenu.visibility. In general however, having multiple handlers for controlling the visibility of admin menu feels a bit overengineered.

Drupal.adminMenu.visbility_settings (note, also a typo in here) should live in a completely separate JavaScript file, as this only needs to be loaded on admin menu's settings page.

Sorry for those partial reviews. I still do not have a clear idea of the intention and purpose of this patch. Based on the recent patches, I think this needs to be simplified. Also, I'm considering to create and commit the changes in #10 in a separate issue (but we should know whether those behaviors should live in Drupal.adminMenu or Drupal.adminMenu.behaviors upfront).

#24

Deciphered - December 17, 2008 - 11:34

@sun

Will change the class, only reason it was .inline-element is that's what it is in AMD as was defined by the previous maintainer.

There are two visibility tweaks currently defined: Key combo and Icon hotspot.
If you look at the Administration menu settings page you can choose between them from the Visibility tweak dropdown. If you haven't got that far, you should be able to see evidence of them in the admin_menu_admin_menu_tweak() (implementation of hook_admin_menu_tweak) or in visibility.js as Drupal.adminMenu_visibility.key_comobo and Drupal.adminMenu_visibility.icon_hotspot.

As for changing the namespace, that's probably not a bad idea, I specifically had to go outside of the namespace so that the visibility tweaks wouldn't automatically get run.

No issue with your reviews, it's your module and I can understand the need to control what goes in. I just want to get the great functionality of AMD in so that people, myself included, no longer need to install both modules to get the combined functionality.
The only way I can think of simplifying things would be to remove the modular nature, but I would be reluctant to do so. While it may be unlikely that anyone will want to add another visibility tweak, or a standalone tweak, Drupal is all about providing that ability.

#25

stanislav.mueller - January 6, 2009 - 10:37

Hi Deciphered,

I've fixed issues you noticed.
We have got new key/combo this is: SHIFT+SPACE (The same combo as browser's scroll to top)

There are some very minor changes in the admin_menu.inc, needed to get right keyboard order during the keyboard navigation.

This patch goes on top of the current admin_menu dev release: 6.x-1.x-dev and is located here: http://drupal.org/node/331982#comment-1183188

Best,
Stanislav

#26

sun - February 12, 2009 - 14:50

Moved my patch from #10 into a separate issue for now: #349505: Performance: Cache menu tree, since we need this registry for admin menu behaviors for other stuff now.

If anything is wrong with that approach or registry structure, which might be incompatible with further features of Administration menu dropdown, please follow-up (quickly) over there.

#27

sun - February 12, 2009 - 14:50

#28

sun - February 12, 2009 - 18:41
Version:6.x-1.x-dev» 6.x-3.x-dev

#29

Deciphered - April 6, 2009 - 04:35
Status:needs work» needs review

Let's try this again:
Patch has been re-written from scratch, and functionality has been stripped down as much as I will willingly sacrifice.

 

What does this patch do?
This patch adds an item to the 'Advanced settings' menu to enable and define a hotkey that can be used to toggle the Administration Menu visibility.

What are the benefits of hiding the menu?
Hiding the menu has many benefits, including:
- Stops menus popping up when the user scrolls to the top of the browser.
- Removes an unnecessary element from the screen when showcasing your design.

What was not included from Admin Menu Dropdown?
As only the bare minimum functionality was included, quite a lot of features where stripped, including:
- Maintaining the previous visibility state when browsing through your site.
- The ability to disable/toggle the fixed position so users can scroll down for longer menus.
- Alternate and extensible visibility behaviors.

 

If I have not provided enough information, please let me know ASAP so that I we can finalize this issue, it really is a pain for myself, and presumably others, to have to install a second module for such a simple extra feature.

Cheers,
Deciphered.

AttachmentSize
admin_menu-DRUPAL-6--2-226012-admin_menu_dropdown-6.patch 4.14 KB

#30

dbabbage - April 8, 2009 - 22:15

What was not included from Admin Menu Dropdown?
As only the bare minimum functionality was included, quite a lot of features where stripped, including:
- Maintaining the previous visibility state when browsing through your site.

Removing this is a huge regression IMHO. It should certainly be kept in any merge.

#31

Deciphered - April 8, 2009 - 22:32

@dbabbage

I also agree, out of the three items mentioned I believe it is the most important to be in there. But my first priority is to get anything in there, then once the core functionality is in there it can be expanded upon with further patches.

The simpler the patch, the more likely it will actually get committed.

Cheers,
Deciphered.

#32

sun - April 8, 2009 - 23:03

The simpler the patch, the more likely it will actually get committed.

Uhm. No. The more rock-solid, future-proof, fleshed-out, and awesome the patch, the more likely it will get committed.

+  $form['tweaks']['admin_menu_tweak_visibility'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Toggle menu visibility with hotkey:'),
+    '#default_value' => variable_get('admin_menu_tweak_visibility', 0),
+    '#prefix' => '<div class="container-inline form-item">',
+  );
+  $form['tweaks']['admin_menu_tweak_visibility_modifier'] = array(
+    '#type' => 'select',
+    '#options' => array(
+      'ctrl + alt' => 'ctrl + alt',
+      'ctrl + shift' => 'ctrl + shift',
+      'ctrl + alt + shift' => 'ctrl + alt + shift'
+    ),
+    '#default_value' => variable_get('admin_menu_tweak_visibility_modifier', 'ctrl + alt'),
+  );
+  $form['tweaks']['admin_menu_tweak_visibility_key'] = array(
+    '#type' => 'select',
+    '#options' => _admin_menu_theme_settings_keys(),
+    '#default_value' => variable_get('admin_menu_tweak_visibility_key', ''),
+    '#description' => '<br />' . t('If enabled, the administration menu visibility can be toggled on and off using the defined hotkey.'),
+    '#prefix' => ' + ',
+    '#suffix' => '</div>',
+  );

We want to use #prefix and #suffix on a new $form['tweaks']['toggle'] form array level. We don't need a separate checkbox, when the second select list would contain a <None> value (the default). The second select should probably use #field_prefix instead of #prefix. Please avoid manual styling (<br />) at all costs (i.e. just remove it here). The option labels of the first select should be wrapped by t() - I'm not entirely sure whether those keys are called the same world-wide.

/**
+ * Return list of available options for visibility hotkey.
+ */
+function _admin_menu_theme_settings_keys() {
+  $keys = array('' => '');
+
+  $i = 65;
+  while ($i <= 90) {
+    $keys[$i] = chr($i);
+    $i++;
+  }
+
+  return $keys;
+}

I do not understand why we need a separate function here? Try this:
<?php
'#options' => array_merge(array('' => t('<None>')), drupal_map_assoc(range(65, 90), 'chr')),
?>

Additionally, I would like to know what the range of characters 65-90 actually translates into (in a comment).

+Drupal.admin.behaviors.visibility = function (context, $adminMenu) {
+  if (Drupal.settings.admin_menu.tweak_visibility) {
+    $(document).keydown(function(e) {

Should use context instead of document here.

+      // Workaround for altKey bug in jQuery >= 1.2.5
+      evt = evt.originalEvent ? evt.originalEvent : evt;

I'm perfectly ok with adding a dependency on jquery_update for 5.x-3.x. Doesn't matter at all for 6.x+, since it ships with 1.2.6 already.

+      hotkey = eval(Drupal.settings.admin_menu.tweak_visibility_hotkey.replace(/\+/g, '&&').replace(/([a-z]+)/g, "evt.$1Key").replace(/([0-9]+)/g, "evt.keyCode == $1"));

Eval is evil. I don't understand why we can't pass the right settings in the first place, so this code can be simplified?

+    $settings['tweak_visibility'] = $setting;
+    $settings['tweak_visibility_hotkey'] = variable_get('admin_menu_tweak_visibility_modifier', 'ctrl + alt') .
+      (variable_get('admin_menu_tweak_visibility_key', '') ? " + ". variable_get('admin_menu_tweak_visibility_key', '') : '');

This should simply end up in Drupal.settings.admin_menu.hotkey. Only 1 JS setting is required to determine whether we want hotkey support.

#33

Deciphered - April 8, 2009 - 23:24

We don't need a separate checkbox, when the second select list would contain a value (the default).

The <None> value implies that you don't want an extra key in the hotkey, ie. you just want Ctrl+Alt as the hotkey oppose to Ctrl+Alt+A.

I'm perfectly ok with adding a dependency on jquery_update for 5.x-3.x. Doesn't matter at all for 6.x+, since it ships with 1.2.6 already.

The comment in the code says >= 1.2.5, as in higher or equal than 1.2.5. The issue was introduced in 1.2.5 and last time I checked it was still in 1.2.6, therefore it would be 6.x-3.x that required jQuery Update not 5.x-3.x-dev.
I will re-confirm that this issue is still present, but I am fairly confident it is still present.

Eval is evil. I don't understand why we can't pass the right settings in the first place, so this code can be simplified?

It's not the settings that is the issue, it is processing the settings as a javascript function instead of a variable. I will look into this to confirm if there is another way, but I can't think of one of the top of my head.

Everything else you say is valid and I will have a new patch up as soon as possible.

 

As for my comment on the simplicity of the patch, I believe it is still valid, if I overloaded the patch with all the current functionality of Administration Menu Dropdown, it would take longer to review and it would likely have more issues.
The simpler the patch, the less there is to review, the more likely it will become "rock-solid, future-proof, fleshed-out, and awesome".

Cheers,
Deciphered.

#34

Deciphered - April 8, 2009 - 23:35

The second select should probably use #field_prefix instead of #prefix.

#field_prefix is, according to the API, only available for textfields:
http://api.drupal.org/api/file/developer/topics/forms_api_reference.html...

#35

sun - April 8, 2009 - 23:39

In correlation with #331982: Add support for keyboard navigation, I also wonder why we don't rely on jQuery to do the ground-work? See http://jshotkeys.googlepages.com/test-static-01.html, resp. jQuery plugin project page http://plugins.jquery.com/project/hotkeys

#36

sun - April 8, 2009 - 23:40

oh my. Sorry about #field_prefix. Sounds like another nice core patch then ;)

#37

Deciphered - April 8, 2009 - 23:49

Please avoid manual styling (<br />) at all costs (i.e. just remove it here)

The <br /> was used due to the inline-container wrapper around the hotkey configuration, without it the description starts on the same line as the select fields. I'm open to suggestions, I would be happy to put it into $form['tweaks']['toggle'] other than the fact that as a markup form type it doesn't support the #description attribute and would require another div wrapper to style it correctly.

 

As for for #35, depending on a jQuery plug-in would appear to over complicate things as we can't package 3rd party code.
IMHO requiring people to download 3rd party code to get access to all functionality limits your user base.

 

#36 sounds good to me.

#38

Deciphered - April 9, 2009 - 00:25

New patch, made as many of the changes requested by Sun in #32 as viable.

Changes not made:
- Checkbox left in as per #33.
- #prefix not changed to #field_prefix as per #34.
- jQuery workaround left in as per #33 - more information available at: http://dev.jquery.com/ticket/2947
- Eval left in as per #33 - will change if a viable solution is determined.
- <br /> left in as per #37.

 

Cheers,
Deciphered.

AttachmentSize
admin_menu-DRUPAL-6--2-226012-admin_menu_dropdown-7.patch 3.9 KB

#39

sun - April 9, 2009 - 01:12

Thanks!

+    '#prefix' => '<div class="container-inline form-item">',

I don't think that " form-item" is necessary in here.

+      'ctrl + alt + shift' => t('ctrl + alt + shift')

Missing trailing comma.

+    // Note: 'range(65, 90)' is used to generate characters 'A' through 'Z'.

Please cut this down to: // 65-90 maps to A-Z (also without "Note:" - comments always note something or should not be there at all ;).

+      evt = (e) ? e : window.event;
+      hotkey = eval(Drupal.settings.admin_menu.hotkey.replace(/\+/g, '&&').replace(/([a-z]+)/g, "evt.$1Key").replace(/([0-9]+)/g, "evt.keyCode == $1"));

Two variables defined in the global scope - please always use var.

+      // Workaround for altKey bug in jQuery >= 1.2.5

If this is a known bug and we just wait for a fix, then why not directly this?
+      // Workaround for altKey bug in jQuery >= 1.2.5 < 1.3.2
+      // @see http://dev.jquery.com/ticket/2947

.

+    $settings['hotkey'] = variable_get('admin_menu_tweak_visibility_modifier', 'ctrl + alt') .
+      (variable_get('admin_menu_tweak_visibility_key', '') ? " + ". variable_get('admin_menu_tweak_visibility_key', '') : '');

If admin_menu_tweak_visibility_key is optional, then we can do this a bit cleaner:
+    if ($hotkey = variable_get('admin_menu_tweak_visibility_key', FALSE)) {
+      $settings['hotkey'] = variable_get('admin_menu_tweak_visibility_modifier', 'ctrl + alt') . ' + ' . $hotkey;
+    }

...and additionally make sure the 'hotkey' property is properly initialized in Drupal.behaviors.adminMenu().

#40

Deciphered - April 9, 2009 - 01:31

I don't think that " form-item" is necessary in here.

I did this because I noticed that container-inline removed the css margin, while it's not an issue with no other items below it at the moment, if there was later there would be no spacing between them.

(also without "Note:" - comments always note something or should not be there at all ;).

Based this on the first comment I found above that area of code. Normally I wouldn't use 'Note:', but was trying to keep things consistent.
Will change.

If admin_menu_tweak_visibility_key is optional, then we can do this a bit cleaner:

$settings['hotkey'] still needs to be defined if there is no 'key', only a modifier, so it would actually turn:

+    $settings['hotkey'] = variable_get('admin_menu_tweak_visibility_modifier', 'ctrl + alt') .
+      (variable_get('admin_menu_tweak_visibility_key', '') ? " + ". variable_get('admin_menu_tweak_visibility_key', '') : '');

into:
+    $settings['hotkey'] = variable_get('admin_menu_tweak_visibility_modifier', 'ctrl + alt');
+    if ($hotkey = variable_get('admin_menu_tweak_visibility_key', FALSE)) {
+      $settings['hotkey'] .= ' + ' . $hotkey;
+    }

While it's cleaner, it's also two lines longer.
If you think it's necessary I will certainly change it.

 

Making other changes now.

Cheers,
Deciphered.

#41

Deciphered - April 9, 2009 - 01:41

New patch.

Only change I did not make, but will pending confirmation, is the 'form-item' class issue.

AttachmentSize
admin_menu-DRUPAL-6--2-226012-admin_menu_dropdown-8.patch 4.14 KB

#42

Deciphered - April 9, 2009 - 03:35

And again:

This patch should make you happy Sun, no more evil eval()!!!
Read through the source code of the jQuery plug-in you provided earlier, and while it didn't directly solve the issue, it helped me re-arrange my thinking and work backwards to get the perfect solution.

Enjoy :)

Edit: There is a bug in this code :(
Working on a new patch now.

AttachmentSize
admin_menu-DRUPAL-6--2-226012-admin_menu_dropdown-9.patch 4.17 KB

#43

Deciphered - April 9, 2009 - 04:12

New Patch, bug fix for #42.

AttachmentSize
admin_menu-DRUPAL-6--2-226012-admin_menu_dropdown-10.patch 4.17 KB

#44

Deciphered - April 9, 2009 - 04:15

Looks like my brain is due for a reboot, real patch attached this time...

AttachmentSize
admin_menu-DRUPAL-6--2-226012-admin_menu_dropdown-10.patch 4.27 KB

#45

sun - April 18, 2009 - 02:32
Status:needs review» needs work

+      'ctrl+alt' => t('Ctrl + Alt'),
...
+    '#default_value' => variable_get('admin_menu_tweak_visibility_modifier', 'ctrl + alt'),

With or without spaces?

#46

Deciphered - April 19, 2009 - 05:35
Status:needs work» needs review

Hi Sun,

That's what happens when you change something at the last minute. My bad.
Fixed it, updated the javascript a bit and added uninstall code.

Cheers,
Deciphered.

AttachmentSize
admin_menu-DRUPAL-6--2-226012-admin_menu_dropdown-11.patch 5.03 KB

#47

Deciphered - April 27, 2009 - 00:42

Re-rolled patch for latest updates to DRUPAL-6--3.

Edit: I don't believe the patch actually needed to be re-rolled as it appears it would have still applied cleanly, just an act of habit.

AttachmentSize
admin_menu-DRUPAL-6--2-226012-admin_menu_dropdown-12.patch 5.01 KB

#48

Deciphered - May 1, 2009 - 01:51

Added patches for HEAD and DRUPAL-5--3.
Patch from #47 is still relevant for DRUPAL-6--3.

AttachmentSize
admin_menu-DRUPAL-5--3-226012-admin_menu_dropdown-12.patch 5.08 KB
admin_menu-HEAD-226012-admin_menu_dropdown-12.patch 4.91 KB

#49

Deciphered - May 29, 2009 - 05:18

Time for an updated patch, only for DRUPAL-6--3 at this stage.

Changes in this patch:
- Added position toggle hotkey - Users can toggle the 'fixed' positioning so they are able to view menu items that are below the screens threshold.
- Added persistent states via cookies - Position and Visibility states are carried across when browsing through your Drupal site.

 

Looking forward to feedback.

Cheers,
Deciphered.

AttachmentSize
admin_menu-DRUPAL-6--3-226012-admin_menu_dropdown-13.patch 8.95 KB

#50

Deciphered - June 14, 2009 - 23:31
Status:needs review» needs work

Due to the drastic changes in Admin Menu 6.x-3.x-dev, a new patch will be available as soon as I have time.

Cheers,
Deciphered.

#51

mcrittenden - June 17, 2009 - 20:02

Subscribe.

#52

mcrittenden - August 26, 2009 - 19:15

sun: any chance of this for the 6.x-3.0 release? (assuming #550254: Menu links are sometimes not properly re-parented finally gets committed)

 
 

Drupal is a registered trademark of Dries Buytaert.