Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The admin_menu provides 'shortcuts' but they aren't really expandable. Why not re-use what's available via shortcut.module? :)
Comment | File | Size | Author |
---|---|---|---|
#76 | admin-menu-broken.png | 6.7 KB | klavs |
#74 | selecting_house_button.png | 3.18 KB | JSCSJSCS |
#74 | reduced_browser_window_overlap.png | 6.94 KB | JSCSJSCS |
#59 | admin_menu.shortcut.59.patch | 8.77 KB | mlncn |
#56 | admin_menu.shortcut.56.patch | 8.86 KB | sun |
Comments
Comment #1
Dave ReidComment #2
sunHm. I'd rather prefer something like this - re-using existing functionality.
However, it's clear from the patches that both admin_menu and Shortcut/Toolbar are not fully prepared for integration yet.
Comment #3
klonosYou guys are talking about the Keyboard shortcut utility module. Right?
subscribing anyways...
Comment #4
klonos...never mind. I just realized this is a D7 core module.
Comment #5
cindyr CreditAttribution: cindyr commentedPlease work on this! I love admin menu, but that shortcut bar is so handy too, and I'd really like to see the ability to use both (seeing the shortcut toolbar under the admin menu like it already works with the Drupal toolbar). I'm afraid to patch the shortcut toolbar, as I doubt Drupal core would be willing to make your patches, wouldn't they? What can I do to help? I'm new at module development, but I'm willing to try...
Comment #6
callison CreditAttribution: callison commented@sun: Would you please explain what is wrong with Dave Reid's approach. I'm assuming what you mean by existing functionality is that in his solution he went through each link adding them to the array instead of using the pre_render approach as in your solution. While yours does seem better in the long-run, I wonder if it would be better to go ahead and use Dave Reid's approach so that we won't have to wait for the Shortcut & Toolbar modules to change first. Later, if they have changed, we could always redo it in a more efficient way. Thoughts?
BTW, I changed the priority to major because this is a pretty integral part of having shortcuts in the first place. If we're going to have shortcut functionality, it needs to work as the original, right?
Comment #7
jsenich CreditAttribution: jsenich commentedsubscribing
Comment #8
stuwat CreditAttribution: stuwat commentedIntegration of the shortcuts with the admin_menu would be an obvious step forward in the development of this module. I look forward to it.
Comment #9
callison CreditAttribution: callison commentedI have re-rolled Dave Reid's patch from #1 against the latest 7.x dev version (see my note in #6). I also added the 'Edit Shortcuts' link on the right side as in the original toolbar drawer from the Toolbar module. Please review and advise.
Comment #10
callison CreditAttribution: callison commentedComment #11
callison CreditAttribution: callison commentedSorry about that - something weird happened. Hopefully this attachment will work correctly.
Comment #12
callison CreditAttribution: callison commentedI made some more changes to the above patch. First of all, it had some errors. The cache clearing was in the wrong place and was causing issues, so it now works properly.
I removed the styling for adding toolbar.png in the background of the shortcut links. I didn't see where that was being used in the shortcuts, and it was causing some weird issues.
I added functionality for the shortcut bar to stay open (at least until the user refreshes or clicks a link) when the toggle button is clicked. It even stays open while the other menu items are dropped down. I think a next step for this functionality would be to save the open state of the shortcut bar in a cookie or something (similar to how the toolbar module does it).
I look forward to your review and thoughts on how I did this.
Comment #13
stuwat CreditAttribution: stuwat commentedLooks great. New items that are added to shortcuts don't appear there until after coming out of the administrative interface, but I suspect that's an issue with the shortcut module. The open state also needs a bit of attention, as you said. On the whole, this is exactly what was needed. Thanks.
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedI think we need to do this the way @sun outlined above. The patch in #12 already requires more knowledge of shortcut module internals than is ideal, and will require more in order to work correctly (for example, currently it displays disabled shortcuts in addition to enabled ones, which is not correct).
So, I've written a patch (attached) that is based on @sun's approach, although also borrowing some of the toggling behavior from @callison's patch above.
To clarify, this approach does not require any changes to Drupal core. (I think above @sun was just saying that the core patch would be a good idea in its own right, i.e. to improve the integration more generally, that's all.)
The attached patch also fixes the caching issues described in #13 (where new shortcuts do not appear immediately when inside the overlay), which was definitely an admin menu bug; Drupal core does not have this problem. Actually it was a bug with the admin menu more generally, although it's more noticeable with the shortcuts since you are much more likely to change those than you are to change e.g. the items in the top menu. Doing the overlay integration properly was a bit tricky and involved fixing some related bugs as well, so that's responsible for the size of this patch. But it should all work correctly now.
I also added some code to make the vertical displacement work correctly when the shortcut bar is enabled (so the main page content is not covered up).
Remaining issues:
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedI also added a link to @sun's core patch from #2 to this issue: #724782: Clean up the shortcut module's CSS
Comment #16
scott.whittaker CreditAttribution: scott.whittaker commentedSubscribing
Comment #17
acoustika CreditAttribution: acoustika commentedsubscribing
Comment #18
Shadlington CreditAttribution: Shadlington commentedSubbing
Comment #19
pjcdawkins CreditAttribution: pjcdawkins commentedSubscribing
For those wanting to use both Shortcut and Admin Menu at the moment, you could extend the admin theme to provide a bar at the top for the Shortcut block. It would only work within that theme though.
Comment #20
kriskhaira CreditAttribution: kriskhaira commentedI ran the patch at #14 on admin_menu-7.x-3.x-dev and got the following error. Also didn't make a difference to my Toolbar and Shortcut despite clearing my cache. I only ran the #14 patch. Is there anything I missed?
Comment #21
klonos@kriskhaira: ...looks like some lines in the code might have been moved around. Try applying the patch manually Kris.
Comment #22
mstrelan CreditAttribution: mstrelan commentedsubscribe
Comment #23
joostvdl CreditAttribution: joostvdl commentedsubscribe
Comment #24
kriskhaira CreditAttribution: kriskhaira commented@klonos: Doing it manually worked. Thanks.
Comment #25
klonosWell it took you a couple of months Kris, but I am glad you finally got it working now ;)
Comment #26
kriskhaira CreditAttribution: kriskhaira commentedWell I did it a few months ago, but realised I didn't leave a note :)
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commented@kriskhaira, if you were able to get the patch to manually apply, would you be able to reroll that as an updated patch file and post it back here? (That way this issue will have an up-to-date patch that people can continue working from.)
If you're already working from a Git checkout you could make the patch pretty easily using "git diff" as described at http://drupal.org/patch/create. If not, but if you're willing/able to spend time on this, we could help you get set up to do it anyway.
Comment #28
Tony Stratton CreditAttribution: Tony Stratton commentedI've applied #14 patch manually from an updated Git checkout. While applying it manually, I noticed a change to this code where it was referenced in the patch:
file: admin_menu.module
patch:
file from checkout:
The attached patch is just an updated #14, but I left the above code in. Should it be removed?
BTW, I'm new at this, so if I did anything wrong, please let me know. Thanks!
Comment #29
james.williams CreditAttribution: james.williams commentedThe attached patch applies against the current master branch and works :-) It's an updated version of the one in comment #14.
Comment #30
james.williams CreditAttribution: james.williams commentedTony - those lines have been replaced with a different fix, which is why the previous patches weren't applying I think. They are to do with a different issue, so probably shouldn't have crept into the original patch in #14. The patch I posted is against the current master branch, so goes alongside the fix for those lines.
Comment #31
james.williams CreditAttribution: james.williams commentedThe attached patch is an attempt at addressing one problem mentioned in #14 - "The admin menu drop shadow appears underneath the top bar only, and the shortcut bar covers it up. Ideally, the drop shadow should be underneath the shortcut bar when it's active (as happens in core)."
Unforunately, this doesn't work in IE currently.
Comment #32
james.williams CreditAttribution: james.williams commentedUsers without the permission to edit their shortcuts don't see the 'Edit shortcut' link - which breaks the styling on the shortcut bar currently (and with the patch I posted above). The attached patch includes the fix for this. Sorry for continually adding additional patches - there may well be a couple more over the next week as I look to address the remaining problems that David outlines with his patch (which my patches are based on) in comment #14.
Comment #33
sunComment #34
David_Rothstein CreditAttribution: David_Rothstein commented@james.williams: Thanks for picking up work on this patch!
Regarding some of the comments above, note that the reason my patch in #14 had this:
Was that it also had this:
In other words, it was just moving that JavaScript from one part of the code to another. Therefore, I don't understand why the patch in #32 only has the second part of that but not the first... seems like we either need both parts removed from the patch or both parts kept in.
I haven't looked at the current code carefully, but I assume we still need both parts kept in, for the reasons mentioned in the above code comment. (And if the current module code isn't using the 'defer' stuff anymore, then we should remove it from the new code as well.)
Comment #35
sunNote: the 'defer' attribute was removed in http://drupalcode.org/project/admin_menu.git/commit/a83b14a96bd78995e62a... - might have to revisit that at some point, but for now, it's out.
Thanks for moving this forward, @james.williams and @David_Rothstein! Will review this patch later.
Comment #36
mefisto75 CreditAttribution: mefisto75 commentedsub
Comment #37
Wolfgang Reszel CreditAttribution: Wolfgang Reszel commentedsubscribe
Comment #38
boreg CreditAttribution: boreg commentedsubscribe
Comment #39
franzkewd CreditAttribution: franzkewd commentedSubscribe
Comment #40
luco CreditAttribution: luco commentedsubs. :]
Comment #41
ralf.strobel CreditAttribution: ralf.strobel commentedAny progress on this would be greatly appreciated. This is some really crucial functionality that's missing.
It should at least be committed to a dev package.
Comment #42
scelza CreditAttribution: scelza commented+1 for committing to dev package
Comment #43
chriz001 CreditAttribution: chriz001 commentedSubscribe
Comment #44
codesmithSubscribe
Comment #45
Dave ReidComment #46
Yura Filimonov CreditAttribution: Yura Filimonov commentedI'm surprised this hasn't been fixed yet, as it's an important part of the website development process and the reason why the admin menu may be disabled or used simultaneously with the core toolbar module (bad, yes, but I need a faster and more convenient access to non-default shortcuts, such as "Create a page").
Thank you for your work.
Comment #47
petsagouris CreditAttribution: petsagouris commentedJust for reference, when this is all done, close #1397872: Admin menu - how to add favourites links? too.
Comment #48
scottrigbymarked #1397872: Admin menu - how to add favourites links? as duplicate of this issue.
Comment #49
mstrelan CreditAttribution: mstrelan commentedPatch in #32 no longer applies. I've recreated this against the latest dev snapshot.
Comment #51
sunok, this is quite a patch... attached one simplifies a couple of things and leverages Shortcut module's existing CSS by faking a div#toolbar into the output.
It still contains a couple of changes from the previous patch, which I'm not happy about, and which require some more in-depth review to understand them.
We're getting really close to a stable 7.x-3.0 release though, and this issue is one of the last remaining blockers.
Comment #52
cinseattle CreditAttribution: cinseattle commentedI tried to apply patch #51 against the current 7.x-3.x-dev branch today, (shortly after it was posted), and am getting errors. Specifically git am results in "Patch format detection failed", and a patch review using tortioseGit results in errors saying that the patch lines do not match.
Am I applying it toward the right branch or do you have any suggestions on how to properly apply this specific patch?
Thanks!
Comment #53
scottrigby@cinseattle #51 applied fine for me with git apply.
@sun & everyone, this patch is so kick ass!
I noticed two things:
Comment #54
sunI've merged and committed the cache flushing improvements from this patch in #282775: Hide disabled menu items
Additionally, #51 lives in a dedicated 7.x-3.x-shortcut topic branch already.
Comment #55
klavs CreditAttribution: klavs commentedI have the same problem as #53. I just tried to install latest from 7.x-3.x-shortcut branch - but it has the same problem.
Comment #56
sunAdditionally, #502408: Move invocation/processing into hook_page_build() just landed.
Thus, re-rolled this patch (and rebased/recreated the 7.x-3.x-shortcut branch) against latest HEAD.
Comment #57
mstrelan CreditAttribution: mstrelan commentedThe padding on this seems unnecessary - at least in the Seven theme and in my custom theme.
Also after enabling shortcut.module shortcut.css wasn't loaded in my aggregated css until a cache flush and therefore Edit Shortcuts overlapped my actual shortcuts, but I don't think this is admin_menu's problem.
Other than that it works well in Firefox 11 and Chrome 17.
Comment #58
Macronomicus CreditAttribution: Macronomicus commentedAwesomeness! There was an error on applying the patch though
Also when hovering over the little arrow that you click to show/hide the bar it disappears.
Other than those small issues it seems to work perfectly!
:) Cheers!
Comment #59
mlncn CreditAttribution: mlncn commentedHere's a re-roll of the patch so that it applies.
* Still loses the drawer arrow when the drawer (shortcut bar) opens.
* Feels like a little more vertical padding than necessary.
* Had to clear cache after adding a new shortcut menu item for them not to print on top of each other.
Comment #60
occupantIt seems that some css has been introduced that really deforms the usability. When the shortcut menu is expanded, the links appear, not on the right hand side of the screen below the arrow / trigger as it used to be, but all the way over on the left hand side of the window. The offending css is in admin_menu_toolbar.css
Line 123
Removing the !important properties from both of the left declarations fixes the issue. I'm not sure what they were intended to correct, so I don't know if that will create problems or not.
Comment #61
sunThe shortcut bar is supposed to appear and behave like it does in combination with the Toolbar of Drupal core. The right-aligned drop-down-menu-alike expansion on hover was a bug, not the intended behavior.
Comment #62
sunComment #64
occupant@sun #61
Really? It's a huge step backward. If a hover-triggered right justified menu is supposed to display it's children full width and left justified, I'd seriously question the decision. It means I have to hover and then scroll from right to left the entire width of my window every time I want to access the items, hoping my cursor doesn't fall outside the hover area (all 33px or so).
If you really feel strongly it should be done this way, it would be nice if you didn't use the !important so it can be easily overridden in theme stylesheets.
Comment #65
sunThanks for reporting, reviewing, and testing! Committed this patch, and a follow-up clean-up and simplification.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
Comment #66
David_Rothstein CreditAttribution: David_Rothstein commented@occupant, I think the idea here is that the hover behavior will be removed entirely. Once you open the shortcuts, they're supposed to stay open.
As far as I remember the earlier patches did this, although the open/closed state did not yet persist between page requests (see #12 and #14). I haven't looked at the newer patches, but if they're back to having the shortcuts only appear on hover then it's possible something got lost along the way.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedOops, crosspost. Well, if there's something still wrong with the behavior, I'm sure we'll find out eventually :)
Comment #68
luco CreditAttribution: luco commentedI agree with @occupant. the way it is, even if it doesn't go away, it's a pain to use. either that arrow should be moved to the left, where it's closer to the links, or the links should be put back on the right, near the arrow that triggers them.
basically you can't have a trigger for a menu that's far from the menu itself. it's not a matter of opinion, it's usability. we're keeping the default look of shortcut.module in a context where it doesn't work just for the sake of looks. that's a bad decision.
Comment #69
scottrigby@luco re 'bad decision', I disagree. this ticket is not to redesign the core toolbar layout, but to integrate with core shortcut functionality
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedThe reason this isn't a usability problem for Drupal core is that the shortcuts stay open across page requests. The arrow is deliberately in an out-of-the-way place, because it's something that is only clicked very rarely.
As I mentioned above, the last time I looked at this patch it did not do that (so you had to specifically open the shortcuts on each page request you wanted to use them). If that's still the case, should a followup issue be filed to fix that?
Comment #71
sunI'm happy to re-open this issue for "simple" follow-up fixes. Larger design/UI changes (that possibly diverge from the Toolbar/Shortcut experience in core) should be properly proposed in separate issues (using mockups, annotated screenshots, or even patches, you know... ;)).
However, most of the discussion in this issue is based on earlier patches and the committed code and behavior looks entirely different. I spent pretty much an entire day with cleaning up and manually testing the new code. It's in no way comparable to any patches or previous behavior you might have tested before.
Thus, before discussing any hypothetical UI/UX issues any further, please make sure that you're running and testing the latest 7.x-3.x-dev development snapshot first.
Comment #72
luco CreditAttribution: luco commentedyou're right on both topics, Sun. sorry :)
Comment #73
luco CreditAttribution: luco commentedbtw I've tested the newest DEV and at first I missed the shortcuts. but then I turned on shortcut.module and voilà!
Comment #74
JSCSJSCS CreditAttribution: JSCSJSCS commentedDownloaded the may 1st dev version and found two issues:
1. Selecting Home (house icon) the edit links moves to the left and ovelaps other links. This does not happen on other admin menu links, just the home icon.
2.) when using a reduced browser window, there is overlap on the left that covers the edit links link.
Comment #75
sunThanks for testing @JSCSJSCS!
Both issues require larger changes, for which I've created an issue + patch: #1564934: Separate toolbar and dropdown menu markup
Comment #76
klavs CreditAttribution: klavs commentedI checked out the latest shortcut branch - and it still has one major problem for me :(
When I've visited the /admin path (f.ex. /admin/modules) - it removes almost all menu-items and also screws up the shortcut bar a bit.
The menu comes back to normal, if I clear the adminmenu cache.
See attached image.
If you have any ideas, as to why it does this for me (and seemingly not for you) I'd gladly try whatever :)
Comment #77
kitikonti CreditAttribution: kitikonti commented@klavs
I think this is the Same Problem like http://drupal.org/node/1546206?mode=2&sort=2
With the latest dev the problem should be solved
Comment #78
klavs CreditAttribution: klavs commented@kitikonti: but I just (yesterday) pulled the latest from shortcut branch from git. This fix seems to not be commited to shortcut branch then?
Comment #79
JSCSJSCS CreditAttribution: JSCSJSCS commentedThis is an issue in process of resolution. Apply this patch:
http://drupal.org/node/1564934#comment-5962972
I have been testing it and it the issue of the edit shortcut link is fixed but there are still some other issues.
Comment #80
sunSorry, the 7.x-3.x-shortcut branch in the git repo was entirely outdated and obsolete. Forgot to delete that.
The most current code lives in 7.x-3.x now, and thus is also packaged in the latest dev snapshot. It even contains #1564934: Separate toolbar and dropdown menu markup already.
Comment #81
klavs CreditAttribution: klavs commentedhehe - that explains it :)
Installed latest dev instead - much better.