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 module doesn't use js-prefixed classes as described in CSS architecture (for Drupal 9) / Formatting Class Names.
The CSS class names don't follow the OOCSS standards naming.
Current class names
- .toolbar
- .toolbar-bar
- .toolbar-tab
- .home-toolbar-tab
- .toolbar-tray
- .toolbar-tray-vertical
- .toolbar-tray-horizontal
- .toolbar-lining
- .toolbar-menu-administration
- .toolbar-box
- .toolbar-icon
- .toolbar-handle
- .toolbar-icon-escape-admin
- .toolbar-icon-shortcut
- .toolbar-icon-menu
Those CSS class names should be changed to the following.
- .admin-toolbar
- .admin-toolbar__tabs
- .admin-toolbar__tab
- .admin-toolbar__tab--home
- .admin-toolbar__drawer
- .admin-toolbar__drawer.is-vertical
- .admin-toolbar__drawer.is-horizontal
- .admin-toolbar__drawer-inner
- .admin-toolbar__menu-item
- .admin-toolbar__icon
- .admin-toolbar__icon--escape-admin
- .admin-toolbar__icon--shortcut
- .admin-toolbar__icon--menu
The JavaScript functionality must be added in as separate classes. There are two options:
- Add classes with the js- prefix
- Use
data
attributes
Comment | File | Size | Author |
---|---|---|---|
#3 | drupal_toolbar-oocss-and-js-prefix-2419135-3.patch | 27.73 KB | lucastockmann |
Comments
Comment #1
mortendk CreditAttribution: mortendk commentedComment #2
lucastockmann CreditAttribution: lucastockmann commentedComment #3
lucastockmann CreditAttribution: lucastockmann commentedThe proposed solution sounded nice to me, so I did it that way.
Everything works fine expect this:
I tried to replace
toolbar-tray
withtoolbar__tray
and remove it's references in javascript and css. But after all it didn't worked. I can't find any reference, but it breaks anyway.Maybe someone will have a look on this and test?
Comment #4
idebr CreditAttribution: idebr commentedI think there is already an effort to clean up the toolbar css over at #1663198: Clean up toolbar css. Perhaps we can focus our efforts in that issue?
Comment #5
mortendk CreditAttribution: mortendk commentedyes there is as i also commented on it. But its 2 very different patches.
This patch is to fix the existing classes & javascript to make it follow the standards - The other patch is "moving pixels around".
so they are not doing the same things & the chance of slowing both patches down, could become an issue.
Comment #6
LewisNymanComment #7
mortendk CreditAttribution: mortendk commentedisnt this part of the "banana", as this issue is also about moving the js out of the css, so its seperating javascript functionality ?
or am i loading to much into the banana ;)
Comment #8
LewisNymanI thought that it was just part of our CSS standards? Does this issue block banana issues?
Comment #9
mortendk CreditAttribution: mortendk commentednope no banana is blocked - the banana issue with moving the template is in the parent patch & should be ready to go.
But after looking at the toolbar's css we need to rewrite that to eater use .js- prefixes or even better use data-selectors instead, so a theme actually can redesign the toolbar without breaking everything.
Comment #10
LewisNymanSounds good, probably easier to keep this as part of #1995272: [Meta] Refactor module CSS files inline with our CSS standards
Also see: https://www.drupal.org/node/2195695
A few comments on these OOCSS class suggestions:
.toolbar__main
What does main refer to? I actually don't know without looking up the html. Ideally this name should be obvious without having to inspect the element.
.toolbar__lining
Is this like an
__inner
? I think that is more common.toolbar__lining_tray-name
I think this could just be
.toolbar__tray-name
? Or.toolbar__tray
Comment #11
mortendk CreditAttribution: mortendk commentedgood call on the classnames. Whats my intention here is to get the css & js separated completely.
We dont end up not beeing able to change the look of the toolbar, cause the way we wrote it, and then never been able to change it because of politics
About the classnames what might would be even better was to use level-1 / level-2 instead as names for the 2 levels, then we dont have to figure out if people gets the metaphor ? - anyways thats for another issue.
about the admin ui hard to theme, it would be nice if you could weight in on the banan #2 meta issue of what we wanna move to classy, as there seems from developer side to be pushback against moving all admin templates over. The idea seems to be "Why would anybody wanna theme X-admin element".
right now its more important if we can seperate the js from the css, at that point i dont care if its called bob or tray ;)
Comment #12
LewisNymanI've updated the issue summary with all the current classes and what they are for. Next is to update the suggestions.
Comment #13
LewisNymanOk I've updated the suggestions, I've changed the parent component to
admin-toolbar
to prevent clashing with other frontend frameworks. I've added variants for the icons and state changes for the toolbar tray, which I renamed to drawer which seems more common.Comment #14
idebr CreditAttribution: idebr commentedHmm interesting. Do you have any examples of such clashes? A quick Google search turned out fruitless :(
Comment #15
LewisNymanMaterial design was the one that came to mind: http://www.google.com/design/spec/components/toolbars.html
We could include the toolbar in the audit spreadsheet in #2012020: Complete a survey of 3rd party resources to aid with component naming conventions
Comment #16
hass CreditAttribution: hass commentedadmin-toolbar is very bad name. Every css class need to be prefixed with it's module name and not any other.
Comment #17
hass CreditAttribution: hass commentedRemoved invalid admin prefix.
Comment #18
hass CreditAttribution: hass commentedThe only reason why toolbar clash with any other stuff is #1944572: Remove "ul.menu" dependency to prevent theme clashes. Please help reviewing this case.
Comment #19
hass CreditAttribution: hass commentedComment #20
nmillin CreditAttribution: nmillin commentedWorking on this. Google Sheet started to organize the classes with notes from the summary.
https://docs.google.com/spreadsheets/d/1DMrCUe1WqTcReaDdZaA8JK8gS8OuQ1sp...
Comment #21
LewisNymanThis isn't the case, our CSS standards does not require us to prefix the classes by the module name, just the name of the component. Modules can define name components, for example the system module defines loads of components like
action-links
andmenu
Comment #22
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedComment #23
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedBecause of a lot of the same code, we discussed in DrupalCon Barcelona, that we are going to reroll in #2452343: toolbar add in javascript classes to remove confusion with css so we have a working patch over there, then close that ticket, and merge it in this one, so we can base all the changes suggested by @lewis upon the rerolled patch mentioned before.
Comment #24
hass CreditAttribution: hass commented@LewisNyman: Why refuse you to understand that such generic named classes may be used in css frameworks? This may disallow adoption of Drupal. We had very good and safe rules in past that everything need to be prefixed and it was good as it has not clashed with any other components or only in rare edge cases. What you say has caused hundreds of clashes in Toolbar. That is why we prefixed all toolbar classes with toolbar and this need to be kept. If we name it prefixing with module or component - I do not care, but never use generic and short names like icon, menu and such things.
Please try to learn from the issues #1938044: Prefix all toolbar classes to prevent theme clashes and #1944572: Remove "ul.menu" dependency to prevent theme clashes what happens with generic classes like "menu", "icon" and other sh**. Re-think and come back and ask if you have not understood these issues, please. But please don't ignore them and tell us anything else.
Don't wait 2 months again before you come back and comment again the wrong ideas, please.
Comment #25
LewisNyman@hass I am not saying that we shouldn't prefix them. I'm saying we should prefix them with
admin-toolbar
instead oftoolbar
. Which one do you think is more likely to be used in another framework?Comment #26
hass CreditAttribution: hass commentedI think a
toolbar-
prefix (the component name) is already fine and ends withtoolbar-foo
what should be safe enough. Writingadmin-toolbar-foo
sound really long and I'm not sure what admin should tell me here as the toolbar may not only an admin thing. It could also be used to the public as a normal menu. Some people already asked for this in navbar module.Comment #27
LewisNyman@hass Do you not think that there is a high chance that frontend theme developers will use a class like
toolbar
? Bootstrap uses thenavbar
class and Zurb foundation usestopbar
. The AUI styleguide does use theaui-toolbar
class. It's also an aria role so I feel like some overlap will happen?Comment #28
hass CreditAttribution: hass commentedI think we are fine with the current names. We made them already very unique to prevent clashes. If it still does not fit for one theme we are now able to override it with theming. That was not possible in past when this case was opened. Today we have the toolbar twig template and all is good from my point of view.
I only see one open todo. Remove the unneeded clearfix class from the toolbar. It is causing issues in my yaml themes.
Comment #29
HOG CreditAttribution: HOG at Skilld commentedIn my mind '.admin-toolbar' good way. But now we have many modules which use '.toolbar' in css & js files.
Comment #41
Pemaksa CreditAttribution: Pemaksa commented* { color: green; }
ul, ol, li { color: blue; }
UL.foo, span.bar { color: red; }
Comment #42
apadernoComment #43
apadernoI am removing the last related issue added, since it's not relevant for this issue.