- 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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mortendk’s picture

Issue summary: View changes
lucastockmann’s picture

Assigned: Unassigned » lucastockmann
lucastockmann’s picture

Assigned: lucastockmann » Unassigned
Status: Active » Needs work
FileSize
27.73 KB

The proposed solution sounded nice to me, so I did it that way.
Everything works fine expect this:

--- a/core/modules/toolbar/src/Element/ToolbarItem.php
+++ b/core/modules/toolbar/src/Element/ToolbarItem.php
@@ -82,7 +82,9 @@ public static function preRenderToolbarItem($element) {
         $element['tray']['#wrapper_attributes'] = array();
       }
       $element['tray']['#wrapper_attributes'] += $attributes;
+      // ToDo: Remove class 'toolbar-tray' with all references in css/js.
       $element['tray']['#wrapper_attributes']['class'][] = 'toolbar-tray';
+      $element['tray']['#wrapper_attributes']['class'][] = 'toolbar__tray';
     }

I tried to replace toolbar-tray with toolbar__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?

idebr’s picture

I 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?

mortendk’s picture

yes 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.

LewisNyman’s picture

mortendk’s picture

isnt 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 ;)

LewisNyman’s picture

I thought that it was just part of our CSS standards? Does this issue block banana issues?

mortendk’s picture

nope 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.

LewisNyman’s picture

Sounds 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

mortendk’s picture

good 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 ;)

LewisNyman’s picture

Issue summary: View changes

I've updated the issue summary with all the current classes and what they are for. Next is to update the suggestions.

LewisNyman’s picture

Issue summary: View changes

Ok 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.

.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
idebr’s picture

I've changed the parent component to admin-toolbar to prevent clashing with other frontend frameworks

Hmm interesting. Do you have any examples of such clashes? A quick Google search turned out fruitless :(

LewisNyman’s picture

Material 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

hass’s picture

admin-toolbar is very bad name. Every css class need to be prefixed with it's module name and not any other.

hass’s picture

Issue summary: View changes

Removed invalid admin prefix.

hass’s picture

The only reason why toolbar clash with any other stuff is #1944572: Remove "ul.menu" dependency to prevent theme clashes. Please help reviewing this case.

hass’s picture

nmillin’s picture

Working on this. Google Sheet started to organize the classes with notes from the summary.

https://docs.google.com/spreadsheets/d/1DMrCUe1WqTcReaDdZaA8JK8gS8OuQ1sp...

LewisNyman’s picture

Issue summary: View changes

Every css class need to be prefixed with it's module name and not any other.

This 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 and menu

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned

Because 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.

hass’s picture

This 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 and menu

@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.

LewisNyman’s picture

@hass I am not saying that we shouldn't prefix them. I'm saying we should prefix them with admin-toolbar instead of toolbar. Which one do you think is more likely to be used in another framework?

hass’s picture

I think a toolbar- prefix (the component name) is already fine and ends with toolbar-foo what should be safe enough. Writing admin-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.

LewisNyman’s picture

@hass Do you not think that there is a high chance that frontend theme developers will use a class like toolbar? Bootstrap uses the navbar class and Zurb foundation uses topbar. The AUI styleguide does use the aui-toolbar class. It's also an aria role so I feel like some overlap will happen?

hass’s picture

I 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.

HOG’s picture

In my mind '.admin-toolbar' good way. But now we have many modules which use '.toolbar' in css & js files.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Pemaksa’s picture

* { color: green; }
ul, ol, li { color: blue; }
UL.foo, span.bar { color: red; }

apaderno’s picture

Title: make toolbar module follow the css documentation » Change the used CSS classes to follow the coding standards
Category: Bug report » Task
Issue summary: View changes
apaderno’s picture

I am removing the last related issue added, since it's not relevant for this issue.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.