Posted by casey on January 16, 2010 at 3:39pm
8 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | toolbar.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Toolbar.js is using the Drupal.admin namespace. No other module uses this, nor should toolbar. It can use some more cleaning but lets review this one first to keep things simple.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| toolbarcleanup.patch | 3.43 KB | Idle | Passed on all environments. | View details |
Comments
#1
The patch was almost there but Drupal.behaviors.admin should be Drupal.behaviors.toolbar. Using the admin on the end is a namespace mismatch. The attached patch changes that as well.
#2
Also note, the deeper something is nested in JavaScript the slower it is. By removing the extra layer of referencing there is an ever so small performance improvement.
#3
looks like u got em all in the code, but shouldn't u also change the comments?
/*** Implementation of Drupal.behaviors for admin.
*/
Drupal.behaviors.toolbar = {
/*** Collapse the admin toolbar.
*/
Drupal.toolbar.collapse = function() {
#4
#5
changed comments
#6
+++ modules/toolbar/toolbar.js@@ -1,18 +1,20 @@
/**
- * Implementation of Drupal.behaviors for admin.
+ * Implementation of Drupal.behaviors for the toolbar.
*/
This should either state what the behavior is actually doing, or it simply needs to go. The former is preferred ;)
Powered by Dreditor.
#7
Updated the patch per Suns comments.
#8
Thanks, looks good now - didn't test though. (hope you did ;)
#9
I did test it... works :)
#10
yay \o/
#11
Thanks. This is probably leftover from when yhahn made a Drupal 7 patch from Admin module.
Committed to HEAD.
#12
There are some more strings to be changed in toolbar.module.
Correcting these fixes sticky tables.
#13
#14
Committed to CVS HEAD. Thanks!
#15
Automatically closed -- issue fixed for 2 weeks with no activity.