Move system action-links.theme.css, breadcrumb.theme.css, and button.theme.css to Classy

I had a go on this.
Please review.

Thanks @pjbaert!

I think instead of attach_library we should attach the library via the Classy .info.yml file. This would preserve the old behaviour of loading these files on every page.

I think we are ok to use attach with breadcrumbs since that has its own template, but yes action links and buttons will need to be added in the info file.

Agreed with @LewisNyman: keep it simple and change as little as possible.

Rerolled for #977844: Remove the 'every_page' option for CSS/JS assets: it is confusing, even damaging and I will look into updating it further.

Attached is my understanding of what needs to change here:

• Remove the attach_library from the breadcrumb template
• Add all three libraries to the .info.yml file
• Rename the CSS files to remove ".theme"

Going to try to make that test a bit less of a pain.

Scratch that, not a battle worth fighting at this time.

I carried out the test steps and can confirm the following:

✔︎ Check that the CSS file was deleted from the module and added to Classy
– action-links.theme.css no longer exists in the module and action-links.css now exists in Classy.
– breadcrumb.theme.css no longer exists in the module and breadcrumb.css now exists in Classy.
– button.theme.css no longer exists in the module and button.css now exists in Classy.

✔︎ Check that the references to that CSS file removed from the system module
– References to action-links.theme.css, breadcrumb.theme.css and button.theme.css within system.libraries.yml have been removed.

✔︎ Check that the CSS file is being loaded in the correct places in Classy
– action-links.css, breadcrumb.css and button.css are being loaded within Classy.

✔︎ Check that there are no HTML classes in the module that relies on the theme CSS
– breadcrumb.html.twig within System has no classes that rely on theme CSS.
– I searched the System module folder and found no instances of markup classes related to button or action links.

I'm unsure why the CSS is loaded in Classy the way it is in #14 and then within a different moving CSS issue it is being loaded differently for eg. here... https://www.drupal.org/node/2553451#comment-10297317.

I may of missed something during late night reviews but if someone can explain that part to me in IRC or here, I can get cracking on more reviews tomorrow :-)

Yeah I've been thinking the same thing, we need to get on the same page.

Thanks very much for the review and testing, btw!

I'm very much not clear on what pattern is the "right" one for moving these files.

In system module, most of these component theme CSS files were attached to the theme key of the base library.

However, in some of these moves we're creating new libraries for each CSS file. Some of those are being attached within the template itself, and some are being attached globally. Some are being attached via the base library of Classy, but some in the component section and some in the theme section.

My understanding was that part of the goal in moving these was to keep the load order the same. So I think what would make sense is to put them in the theme section of the base library, unless that's not where they were loaded in core.

But I'm not clear on if that is right or not.

It would be good to write down the rules for which library or libraries should be used for css files moved into Classy, which section of the library and why. Maybe that should go on the meta.

I'd say let's discuss Thursday, but time is really, really short, so if we can make a call before then, I'd say let's do so. We have patches for all the moves now. We just need to pick something consistent, get all the patches lined up the same (not too hard), do visual regression testing on each, then RTBC them and get them in.

I'd call not creating a new library for each CSS unless there is clear reason why to do so. There is not much difference whether we have the new library there or not if its anyway always being loaded. Good thing about not creating the libraries now is that we see quickly which CSS files are always being loaded.

I see, there is reasons to have them in separated libraries. However the altering could only happen in a child themes of Classy where they would be by default loaded so some kind of altering has to be done anyway to remove the dependency. Other reason why we shouldn't do it that way is because most of the issues have been done already the other way so we should stick to that.

And use the theme section of the base library? Not component, right?

I don't think you would load a Classy library from contrib. A module shouldn't be requiring Classy for it's styling, and a contrib theme would either use Classy as a base, in which case it would have the libraries, or it doesn't use it as a base and therefore doesn't want Classy's libraries.

My thinking is to create separate libraries if we think we can conditionally include the css, like how we use attach_library() in the template. If not, and the CSS is loading globally, just put it in base.

The reason to split them up is if we think we'll be able to conditionally add them later and splitting them up would be a BC break. Do we think that? If so, then I understand. However, I think changing the CSS from global to conditional, regardless of whether the library is separate or not, is just as big a change, so that may not matter.

For me the bigger issue right now is whether these should go into component or theme. I know some were being added to theme, because that is where they were added in core/system. The problem is I think we are dealing with a CSS ordering change because it is moved to the theme level. I moved some things to component to get it back to an earlier spot in the list, especially since Bartik adds a lot of things in component. What is important here is to look at the ordering of the files in the markup and try to keep the files in the same relative order when they move to Classy. That is also why some things were aded with a weight.

Also, if we do add things in "theme:" shouldn't the files themselves go into a "theme" folder? Isn't that how we should do things. The folder structure matches the categories.

Ah.... if they were going in the component section so they load higher in order, that makes a lot of sense. Thanks for the clarification, David.

Loading conditionally does make sense for the larger strategy of Drupal 8 performance. Load CSS/JS only when you need it. While currently it's a good practice to minimize server requests, so a global aggregate even when some CSS isn't used makes sense, with http/2, the opposite will be true. Minimizing file size will be more important than minimizing server requests. So for the long lifespan that D8 will have, loading the CSS conditionally will be a better strategy for performance.

I guess I'm most concerned right now about getting things out of core, though. We do not have long to do this, and if it makes it easier to get the CSS into Classy by putting it in the base library, maybe that's the best way to go.

This should address the consistency questions.

I'm concerned how much these drop. CSS added by modules will always be higher, regardless. I don't know if that was by design, or an issue that needs to be addressed.

Otherwise, as close as we can get it.

before

after

No longer applies due to #2553465: Move system exposed-filters.theme.css, more-link.theme.css, and pager.theme.css to Classy.

In fact, these are all going to step all over each other, so should we maybe do all of the moving in one patch?

+++ b/core/themes/classy/classy.libraries.yml
@@ -2,9 +2,12 @@ base:
       css/components/more-link.css: { weight: -10 }
-      css/components/exposed-filters.css: { weight: -10 }
     theme:

Exposed filters are not part of this issue, this shouldn't be in there :-(

Ah I see! But you left the following not in alphabetical order...

css/components/pager.css: { weight: -10 }
css/components/more-link.css: { weight: -10 }

Apologies for confusion and picking at things :-)

The mega was committed so we don't need this.