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 toolbar handler uses menu.link_tree service to create the menu, which does not add the toolbar specific classes. This causes display issues with the devel menu when using default margins and padding for uls versus the resets used in the toolbar menu templates and css. The handler should use the toolbar.menu_tree extension class which adds the necessary templates.
Comment | File | Size | Author |
---|---|---|---|
#9 | devel-use_toolbar_menu_tree_for_toolbar-2881521-9-d8.patch | 3.04 KB | mikelutz |
#7 | devel-toolbar-bartick.jpg | 35.01 KB | willzyx |
Comments
Comment #2
mikelutzComment #3
willzyx CreditAttribution: willzyx commented@mikeNCM thanks for contributing! moving to NR for trigger the testbot
Comment #5
mikelutzSo, my motivation for this change is an issue with the margin in the toolbar menu. My original patch needs a little bit more thought to work across the board. The main issue is that the ul that holds the devel menu needs to have zero top and bottom margin to look right. in bartik, ul.menu is set to 0 top and bottom margin, however in seven, it is given .25em top and bottom margin, and with a custom theme, chrome defaults to 1em top and bottom margin on a ul. Toolbar fixes this in its menus by using the toolbar menu tree, which gives the ul a class of .toolbar-menu instead of .menu. This class is given zero margin in core stable css. The patch in #2 switched devel to use this menu tree, and changed the class from .menu to .toolbar menu.
Good Toolbar:
Devel tray in seven:
Devel tray in custom theme on chrome.
This caused a few additional problems though. First, in bartik, the ul gets a clearfix added, and this combined with the floated right 'configure' option in the devel toolbar means that the css in devel needs to be updated to float the new ul to the left (or right in rtl) or bartik will have the configre button clear the rest of the menu, and end up on a second line.
The second issue is the tests, which are looking for the wrong css class to run. They are easily updated.
So, the two options are to update the patch to fix the tests and the css, which I have done and included here. I think it's the 'right' thing to do to use the toolbar menu tree to create the toolbar menu. The only issue is that it does change the class of the ul, and could potentially break some css on someone's custom theme. I think this is unlikely, and given the fact that devel is primarily used for developers anyway, I don't believe any CSS changes will seriously negatively affect anybody.
The other solution is to explicitly add a margin:0; in the devel css for the .toolbar .toolbar-tray-horizontal .menu selector, which is already there. this makes it a one line change, doesn't change the css class, and may be a better option for those reasons. It's really a question for the maintainers as to how important maintaining backwards-compatible css is.
Comment #6
willzyx CreditAttribution: willzyx commentedComment #7
willzyx CreditAttribution: willzyx commented@mikeNCM Looks good. I think that the change that you suggest is quite reasonable and probably we can commit it.. but the patch is incomplete and cause some visual regressions. For example (as you describe before) the result using bartik is something like
Comment #8
mikelutzBah, my fault, wrong file. It doesn't include the updates to the CSS file. I'll post the right one shortly.
Comment #9
mikelutzThis one should be the correct one.
Comment #11
willzyx CreditAttribution: willzyx commentedCommitted and pushed to 8.x. Thanks and sorry for the delay!