[Meta] Move module.theme.css files to Classy or Seven

So the only CSS classes that we want to retain in modules are those related to javascript states/behaviors, and if any of those classes include both js and "theme" properties, the theme properties should be moved to the Classy theme?

@mducharme: CSS that is either absolutely required for the module's basic functionality (*.module.css), or is directly related to admin pages/tools (*.admin.css), should stay in the module. CSS that just makes the module output look good (*.theme.css) should be moved to classy.

Note that were finding some places where the line between theme and admin is getting blurred. Mostly with things around the toolbar and such.

Important: When moving CSS to Classy that was previously attached in a library containing JavaScript, we can't just move it and attach it in a template. Instead we must implement hook_library_info_alter() to add the new Classy library as a dependency of the existing module library. See the patch in #2489564: Move user.theme.css to Classy for an example of how that's done. That patch will need to land before this approach can be used, since the classy.theme file doesn't exist yet.

Can we get an official naming convention for libraries in Classy? I can't find any actual documentation on this, but based on the way hook_library_info_alter() works it appears that library names need to be unique, so should we namespace them all with "classy."? This is a particular concern when we're adding on to existing libraries.

What will be the naming convention? We should make sure all patches are consistent. Currently we have #2489580: Move taxonomy.theme.css to Starterkit Theme and/or Claro where the library is called taxonomy-theme - and we have #2489564: Move user.theme.css to Classy where the library is called classy.user - and I've seen some other names as well … All of these library names should be consistent. I would say [module].theme

Any other thoughts?

Could it just be "user" so that the library would be referenced as classy/user ?

Look at the way hook_library_info_alter() is implemented:

function classy_library_info_alter(&$libraries) {
  if (isset($libraries['drupal.user'])) {
    $libraries['drupal.user']['dependencies'][] = 'classy/classy.user';
  }
}

You'll notice that the keys in $library do not indicate the module/theme providing the library. In this case, drupal.user is the name defined by the core User module for this library. That tells me that library names must be unique, which tells me that we really, really should be namespacing all our library names. Therefore it only makes sense to prefix them all with "classy." in the Classy theme.

I agree that there is no need to use the word "theme" in any of the libraries we create here. Classy is a theme, so by definition all of these libraries are theme related. The CSS is also being loaded in the theme group, so this is again clear from context.

Names should be short, but descriptive. They should probably use a dot syntax, since that seems to be how most libraries in core are named. Libraries that are intended to extend core module libraries should use the same name, changing only the namespace prefix (core libraries are generally prefixed with "drupal").

I would say look at the docs and example for hook_library_info_alter() instead. There is a second parameter, $extension, that can (and probably should) be checked (a small docs issue is needed to change $module to $extension there, $extension is used when invoking the alter hook).

Seven and Classy should probably be checking $extension, not just the library name. Every other implementation of hook_library_info_alter() in core seems to check $extension.

By that logic the namespacing within an extension is not necessary (the extensions are namespaced already!). I'm not saying don't do it but if it's not necessary let's think a bit more, especially since people will probably go to Classy as an example.

Ahh. That actually makes way more sense now. I was going off the implementation in Seven, but sounds like that needs more work.

If we don't need to worry about name conflicts then this is less of an emergency than I though it was. I still think it's best to develop a standard for these names, but browsing through core right now it looks like the wild west. I see very little consistency. I vote that we keep talking about it, but don't let this bog us down and hold up any issues. For the time being, when moving CSS files over from core modules, why don't we just use the same library name that was used in the original module, minus the "drupal" prefix. So...

drupal.taxonomy --> taxonomy
drupal.user --> user

We can always cleanup the naming conventions once we have a real naming convention, but this will at least let us get the CSS moved over.

Small related docs issue that could use some love (I can review): #2490936: hook_library_info_alter() docs and function signature are slightly out of date

Thanks @Cottser for the information! Glad we figured that out.

Yes, Cottser is correct. There is no technical requirement for library names to be unique. I believe we already have a couple that are repeated. And, for example, every module and them can make a library called "styles". It won't break because there is additional context of what registered the library. But, as jstoller points out, we should make sure to be consistent so that it not only makes sense but can be manipulated/removed in a predictable way.

Is everyone ok with just using the module name for ones that come from a module? Will any have more that one? My only concern would be that the split up system ones would cause a conflict or be misleading somewhere.

as a sidenode i was looking through the modules css & theres multiple files where the markup comes from classy, so it dont make sense to have em inside of the module
Ive created seperate issues for these, so we can fight over them one by one

Things are never easy it seems.

Trying to move some of the discussion from #2489564: Move user.theme.css to Classy here. There is an issue with conditionally loaded libraries (#attached) where Classy's CSS will be loaded after a sub theme's CSS.

Talked to @davidhernandez and @Fabianx in IRC about this issue.

One proposal to solve the loading order would be to implement libraries-extend in theme .info.yml files to be able to include additional assets to, for example, the drupal.user library and have them loaded alongside those assets (rather than abusing the 'dependencies' property in libraries). Keep in mind the .yml is not strictly speaking required to implement this but the direct hook_library_info_alter code might be pretty unwieldy.

@Wim Leers in #2451411-9: Add libraries-override to themes' *.info.yml talks about this. I'd like to create an issue to discuss this further since it's not really coupled to this meta but need to run now.

Dummy YAML for what I'm talking about:

# Extend core's drupal.user library by merging in the assets from classy's user library.
libraries-extend:
  core/drupal.user: classy/user

Here's the issue for libraries-extend: #2497667: Add libraries-extend to themes' *.info.yml

The biggest remaining chunk for this is all the component .theme.css files in system module. We need to create issues for those, broken up into manageable chunks to make testing realistic.

There are already issues for action-links, breadcrumbs and buttons. Here are the remaining theme.css files:

- collapse-processed
- container-inline
- details
- exposed-filters
- field
- form
- icons
- inline-form
- item-list
- link
- links
- menu
- messages
- more-link
- node
- pager
- progress
- tabledrag
- tableselect
- tablesort
- tabs
- textarea

That's 22 files. Separate issues for each one is going to be too hard to move through, but a mega-issue is going to be hard to review. Probably best off if we can chunk these into similar components that will be reasonable to review together.

Classy has already chunked some templates together, so here are some thoughts on how to break this up.

Issue chunks

Content

- node

Dataset

- item-list
- tabledrag
- tableselect
- tablesort

Field

- field

Form

- collapse-processed
- container-inline
- details
- form
- inline-form
- textarea

Miscellaneous

- icons
- messages
- progress

Navigation

- link
- links
- menu
- tabs

Views

- exposed-filters
- more-link
- pager

I'm not sure if that's a perfect breakdown, but I don't think we should spend too much time bikeshedding this if we want to get these in (and we do, because this will be much harder if not impossible after RC1).

Okay to proceed setting issues up with these chunks?

If you are in the mood to make some issue, go for it. I don't care too much about the organization of the issues, as long as they are small enough to do easily. Lately, it seems anything that hits a rough patch stalls for a long time, so small and quick should rule.

New issues:
- #2553447: Move system node.theme.css to Classy
- #2553449: Move system field.theme.css to Classy
- #2553451: Move system tabledrag.theme.css, tableselect.theme.css, and tablesort.theme.css to Classy
- #2553457: Move system collapse-processed.theme.css, container-inline.theme.css, details.theme.css, form.theme.css, inline-form.theme.css, and textarea.theme.css to Classy
- #2553461: Move system icons.theme.css, messages.theme.css, and progress.theme.css to Classy
- #2553463: Move system link.theme.css, links.theme.css, menu.theme.css, and tabs.theme.css to Classy
- #2553465: Move system exposed-filters.theme.css, more-link.theme.css, and pager.theme.css to Classy

Thanks, Marc. Also, see here, #2489566: move file.theme.css to classy , where Lewis and I discussed just moving everything to a components folder.

We might need to discuss/document CSS ordering. I'm not sure if everyone realizes how exactly it is working out. Or if it is even working precisely as it was envisioned.

What about non-system (and even non-module) *.theme.css files like the following (I don't see issues or discussion for these)? Not saying all of these should be moved to Classy but just not want to make sure we discuss. I ignored anything with ".admin." in the name, toolbar, and @mdrummond already covered everything in core/modules/system/css/components.

• core/misc/dialog.theme.css
• core/misc/dropbutton/dropbutton.theme.css
• core/modules/contextual/css/contextual.icons.theme.css
• core/modules/contextual/css/contextual.theme.css
• core/modules/quickedit/css/quickedit.icons.theme.css
• core/modules/quickedit/css/quickedit.theme.css
• core/modules/shortcut/css/shortcut.icons.theme.css
• core/modules/shortcut/css/shortcut.theme.css

And based on #2553461-1: Move system icons.theme.css, messages.theme.css, and progress.theme.css to Classy here's a version without any .icons. files:

• core/misc/dialog.theme.css
• core/misc/dropbutton/dropbutton.theme.css
• core/modules/contextual/css/contextual.theme.css
• core/modules/quickedit/css/quickedit.theme.css
• core/modules/shortcut/css/shortcut.theme.css

So my question is, should some or all of these files be moved to Classy, or should they stay as is because we consider them essential in some way, like Toolbar?

Most of those look like functional theming to me. If somebody doesn't want to use Classy as a base, and want the clean markup core provides, Toolbar and Quick Edit and Contextual Links should all look like Drupal. If somebody really wants to override those, fine, but I don't think most will want to do so.

I say, move 'em all to Classy! Of course I still think we should move all the admin stuff to Classy, so... :-)

Seriously though, the more we leave out of Classy the more potential problems we'll have when trying to update things down the road.

@LewisNyman: Yes, those issues are exactly along the lines of what I'm thinking.

It looked to me like at least some of those files are related to the admin UIs that appear on user-facing pages controlled by the default theme rather than the admin theme. If that is the case, moving those files to Classy would not be good, as then themes that aren't based off of Classy could have problems with how the admin UI looks. But maybe I'm wrong about what's in those files.

It does make a lot of sense that there should be some way for the actual admin theme to control that appearance. It would be neat to figure out how to do that for a future release of Drupal.

contextual.icons.css is the icons for contextual links. Should be available for all themes, not just those based on Classy.

contextual.theme.css is some sort of theming for contextual links. Probably should be available to all themes.

Looks like same is true of quickedit.icons.css and quickedit.theme.css.

Probably the same for the shortcut.icons.css and shortcut.theme.css files.

I'm less clear on what dialog.theme.css and dropbutton.theme.css do and where and how those might be used.

dropbutton is associated with dropbutton-wrapper.html.twig in modules/system.

dialog seems to be associated with ckeditor and views.

So both may still be necessary in core? Not sure.

Bumping to major and tagging RC target - I'm not clear what can and can't be done here once we hit release candidate.

@LewisNyman: CSS in core can also be used in Bartik and Seven. But if we move the CSS that styles things like contextual links to Classy, then themes not based on Classy will need to style contextual links, toolbar, etc. I don't think that's what we want. Admin components on the front end should have default styles. So those should stay in core not Classy.

As a themer, it really is not that big a deal to copy over a few templates from Classy to my custom theme to style these widgets. And honestly, if I'm the kind of themer who's building a custom theme from scratch, bypassing Classy, then what makes you think I want to use Drupal's cruddy old widget theming anyway? The whole point of Classy (well, one of them) was to make things easy for the "burn it all to the ground and build from scratch" themer. I don't think we should be forcing a standard Drupal look on people who may not want it. That's the sort of thinking that earned Drupal the bad rap that all Drupal sites look like Drupal.

Maybe I'm completely off base, but my understanding of the Banana Consensus was that we were moving markup and CSS to Classy that wasn't functional. Front-end admin things like contextual links were specifically excluded from that.

Part of the whole point of Classy was to make it much easier for people to not use Drupal's default markup when creating a theme. But if I choose to do so, and that means that I either need to re-imagine all the styles for the admin toolbar, contextual links, quick edit, etc., every time I make a non-Classy theme? That significantly increases the difficulty of creating non-Classy themes. Yes, you can "just" copy over all those stylesheets, but you need to know which ones to copy over, and it's one extra step.

I think the number of people who really want to redo those authenticated front-end admin tools is very small. I certainly don't have the time or inclination to do that. Mind you, you still *could* do that if those theme CSS files are still in core. You just don't *have* to do that if you don't want to do so.

The original Banana Consensus called for taking a slash and burn approach and moving everything we possibly could to Classy. That way you could drop in [front-end framework of the week] and build your own theme from scratch, without Drupal core getting in your way. Meanwhile, Classy would provide some basic common sense defaults for the majority of themers to use and build on. The whole admin/front-end differentiation happened after the fact. The custom theme crowd eventually capitulated to pressure from Core developers and agreed not to move "admin stuff" to Classy, in order to prevent a complete collapse of this effort. However, that has caused some continuing conflict around the margins, like what we're seeing here.

In addition to supporting the hardcore themer crowd, Classy was also intended to provide more flexibility on the front-end, without needing to break backwards compatibility. We talked about being able to release a Classy2 some months/years down the road, completely changing the base theme to use updated best practices, while leaving the original Classy as is, so no one's themes would break. To support this use, we need to have as much stuff in Classy as possible. If you leave the theming for some widget in a Core module then that code becomes untouchable for the life of D8, which on the front-end is a tragedy.

Toolbar assets should remain in core. The toolbar, and functional components like it, should continue to function properly without Classy.

Contextual links and the toolbar are rendered both in the front-end and admin themes, so how would they be styled by Seven when the current theme is Bartik?

Right, exactly. Seven does not have the ability to output CSS on a front-end page when Bartik or anything else that isn't Seven is the default theme. I agree it would be good if that were possible, and that would solve this whole debate, but that's definitely not the case right now.

The nice thing is there are better tools now like stylesheets-remove (or hopefully soon #2451411: Add libraries-override to themes' *.info.yml) for people to make these decisions for themselves.

As long as there are tools to remove this (partially?) "functional" styling I think it's fine to include it in core. I think what we are talking about is already split up by library because it's coming from optional core modules (at least for toolbar and contextual) so it should be fairly easy to strip out if it's not wanted. This brings up another important point - there is still no way for contrib modules to differentiate their CSS (or markup) for Classy/Stark (aka core) so in a way I think it's good to have some core modules that do ship with their own CSS (in library form) enabled by default.

From #41:

If you leave the theming for some widget in a Core module then that code becomes untouchable for the life of D8, which on the front-end is a tragedy.

That would indeed be tragic but unless I'm missing something I don't think that's the case – we can still copy core things to Classy even past release. The worst case scenario is if you're rolling with core only then you need override or remove a widget's library and plug in your own styling. It's still flexible.

Edit: What I think would be potentially interesting to do here would be see if it's possible to actually take out some of the visuals from things like toolbar (remove all color declarations and only keep basic layout and whatever showing/hiding is needed, for example) but still keep them functional.

Edit 2: Definitely a crosspost but I stand by it :)

...we can still copy core things to Classy even past release.

I remember asking about this during the Core sprint in LA and @alexpott was pretty adamant that anything coming out of a core module (or Classy, or any core theme) was API and thus couldn't be changed down the road. I know there's been talk about easing up on this a bit and I'm a little behind on that conversation, so if this hard-line stance has changed then I'll do a little happy dance and stop pushing for everything to go into 8.0.

See #2544740: [policy] Consider not supporting core module markup and CSS between minor releases where this is being discussed.

I noticed something. Bartik and Seven both have CSS folders that mimic the SMACSS categories; base, layout, theme. But both have a components folder, plural with an 's'. I created the same in Classy, copying the others. Was the 's' a mistake? Should they all be 'component', singular?

@davidhernandez that is a good point, made me look at my own project trying to follow SMACSS. I copied from Omega 4's interpretation:

base               
components         
layouts 
modules

bases doesn't work... but component/ does maybe without the plural would make most sense for core?

VOLTRON PATCH!

Lools. :D

Yes, lets mega patch it all. The system ones should be easy since they all go into the base library, and there is no conditional inclusion. They shouldn't need to touch templates or anything else.

Also, where ever the patch ends up lets make sure to pull in anyone that worked on the other issues so they can get commit credit.

The system *.theme.css issue has been committed.

Moving to 8.1.x, and we have a plan for the remaining files via Stable.

• Issue summary needs updating as it is targeting themes that have been removed
• It should be determined if this is even still desirable because:
  • There is now libraries_extends and libraries_override
  • Enough time has passed that this might not be desirable for Drupal anymore. This needs to be verified before moving further
  • Even if there are benefits to the approach, we also have to recognize that it would be disrupting something that has been in place for nearly 10 years. Are the benefits worth that disruption?

There's following files in 10.1 core

./core/themes/stable9/css/toolbar/toolbar.icons.theme.css
./core/themes/stable9/css/toolbar/toolbar.theme.css
./core/themes/stable9/css/contextual/contextual.icons.theme.css
./core/themes/stable9/css/contextual/contextual.theme.css
./core/themes/stable9/css/taxonomy/taxonomy.theme.css
./core/themes/stable9/css/views_ui/views_ui.admin.theme.css
./core/themes/stable9/css/content_moderation/content_moderation.theme.css
./core/themes/stable9/css/settings_tray/settings_tray.theme.css
./core/themes/stable9/css/shortcut/shortcut.theme.css
./core/themes/stable9/css/shortcut/shortcut.icons.theme.css
./core/themes/stable9/css/update/update.admin.theme.css
./core/themes/claro/css/theme/views_ui.admin.theme.css
./core/themes/claro/css/theme/toolbar.icons.theme.css
./core/themes/claro/css/theme/filter.theme.css
./core/themes/claro/css/theme/tour.theme.css
./core/themes/claro/css/theme/toolbar.theme.css
./core/themes/olivero/css/theme/filter.theme.css
./core/modules/toolbar/css/toolbar.icons.theme.css
./core/modules/toolbar/css/toolbar.theme.css
./core/modules/system/tests/modules/css_disable_transitions_test/css/disable_transitions.theme.css
./core/modules/contextual/css/contextual.icons.theme.css
./core/modules/contextual/css/contextual.theme.css
./core/modules/taxonomy/css/taxonomy.theme.css
./core/modules/views_ui/css/views_ui.admin.theme.css
./core/modules/content_moderation/css/content_moderation.theme.css
./core/modules/settings_tray/tests/modules/settings_tray_test_css/css/css_fix.theme.css
./core/modules/settings_tray/css/settings_tray.theme.css
./core/modules/shortcut/css/shortcut.theme.css
./core/modules/shortcut/css/shortcut.icons.theme.css
./core/modules/update/css/update.admin.theme.css
./core/profiles/demo_umami/themes/umami/css/components/tour/tour.theme.css

I have to assume the list of files in #81 are there to provide information that moves the issue forward, but I'm unfortunately not sure what that is.

Since both themes have been removed should this be closed.

#2489460: [Meta] Move module.theme.css files to Classy or Seven is open from the child issues, and was rescoped to Claro.

It's not clear to me if the child issues represent all the intended work here or not. If they do, we've got one or two left and it could be closed. If it doesn't, then the entire issue summary needs a rewrite with clear next steps.

So should this be closed?