Move user.theme.css to Classy

If this is the case then perhaps we need to rename user.icons.theme.css to user.icons.admin.css?

Yes

The user.theme.css file is a bit more complicated. The markup being targeted by this CSS appears to be coming from user.js, rather than from a template file. Plus, there is already a user.admin.css file and the user.module.css file contains basic styling so the password strength indicator will work.

I think if the styling is clearly theme styling, like the colours for the password indicator, then we should move it to Classy. We can create a followup issue to decide what to do about classes added in Javascript, in this situation it feels like the module is just completing the UI component by adding state classes. I think it's not that bad to have them added in the module but not styled in the module for the time being.

@jstoller: I'm not sure if there is going to be working solution for that other issue anytime soon. As far as I've seen It's quite complicated and there is no work done yet to make it happen. Seven has similar PHP code in seven.theme file so I think it could be copied and used for this purpose. Not optimal but probably necessary at this point.

@jstoller thanks for all this. It seems to me like the code is working as expected. I'm fairly sure you need to clear caches because the library definitions are cached.

Seems to work for me too!

@jstoller: could you check that your git configuration aligns with the standards that are given here: https://www.drupal.org/documentation/git/configure

The problem is on the patch. With the git config that we suggest to use the patch will became a little bit easier to read :)

reroll

lets try this time with a file

screenshots:
seven:

classy:

Bartik:

+++ b/core/themes/classy/classy.libraries.yml
@@ -3,3 +3,9 @@ base:
+classy.user:

I created a comment to discuss the naming convention for the libraries. I don't feel like we should add the classy. before user, since you're now including classy/classy.user which seems pointless to me. I suggest user.theme, but let's discuss that in the comment.

Please use $extension, I created #2490936: hook_library_info_alter() docs and function signature are slightly out of date to fix the docs :)

Other than that, looks good!

I missed this before, but the patch no longer deletes the user.theme.css from user module ;) #mortendkblaming

assign blame !
rerolled the patch

lets try this again and also rename icons.theme

As a follow up as its out of scope (so DONT add it to this patch)
We should look at what to do with the user.admin.css its all css that should live over in seven #2491149: move user.admin.css into seven where it belongs

There's something going on where visually it looks slightly different in Seven (less spacing underneath the Password strength), I did notice that the Classy CSS is being loaded at the very end which seems wrong.

Does this have anything to do with #2474091: CSS files from themes are being loaded in reversed order or are we just using it wrong somehow?

Before:

With #21:

<style media="all">
@import url("http://d8.dev/core/themes/seven/css/base/elements.css?0");
@import url("http://d8.dev/core/themes/seven/css/base/typography.css?0");
@import url("http://d8.dev/core/themes/seven/css/base/print.css?0");
@import url("http://d8.dev/core/themes/seven/css/layout/layout.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/jquery.ui/theme.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/admin-list.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/content-header.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/breadcrumb.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/buttons.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/colors.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/messages.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/dropbutton.component.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/entity-meta.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/field-ui.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/form.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/help.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/menus-and-lists.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/modules-page.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/node.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/page-title.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/pager.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/panel.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/skip-link.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/tables.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/system-status-report.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/tabs.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/tour.theme.css?0");
@import url("http://d8.dev/core/themes/seven/css/components/views-ui.css?0");
@import url("http://d8.dev/core/themes/classy/css/user/user.theme.css?0");
</style>

Theres defently something going on with the cascade - THIS IS WHAT HAPPENS WHEN YOU WRITE SHITTY CSS PEOPLE
... ok time to debug ;)

Even moving the user.theme.css added to Classy in this patch to the 'component' group which is used in most of Seven puts it below all the Seven stuff. Why is that?

And I don't want to derail this issue too much but shouldn't making a library containing a CSS file from core depend on a library containing a CSS file from Classy make the Classy CSS come first? Certainly I would expect/hope JS dependencies to work that way anyway…

I have no idea classy should be loaded first right
order:

1. Modules
2. basethme (classy )
3. theme (seven)

I would think so. Going to try and find some time tonight to poke around at the libraries stuff and see how it's designed.

@Cottser: See #2389203-57: Validate the CSS categories in libraries.yml files.

I have no idea classy should be loaded first right

CSS Loading order

Kitten is a sub theme that has base theme: classy.

The loading order will be

Base styles from Classy, then from Seven/Bartik/Kitten. (Weight: -200)
Layout from Classy, then from Seven/Bartik/Kitten. (Weight: -100)
Component from Classy, then from Seven/Bartik/Kitten. (Weight: 0)
State from Classy, then from Seven/Bartik/Kitten. (Weight: 100)
Theme from Classy, then from Seven/Bartik/Kitten. (Weight: 200)

Needs documentation?

I don't know where the documentation for this is. If there is none, I'll improve the example above and add it to the docs.

classy.libraries.yml

base:
  version: VERSION
  css:
    component:
      css/user/user.theme.css: {}
    theme:
      css/layout.css: {}

Like this it works. Maybe it's because it's done in an alter hook? Good luck digging into the code Cottser!

This may have something to do with Seven's global-styling library being loaded on every page, therefore it comes first. Any library that isn't loaded on every page seems to come after, possibly because of Drupal\Core\Asset\AssetResolver::sort() - see the inline comments there:

// Within a group, order all infrequently needed, page-specific files after
// common files needed throughout the website. Separating this way allows
// for the aggregate file generated for all of the common files to be reused
// across a site visit without being cut by a page using a less common file.

So this kinda works as expected:

user:
  version: VERSION
  css:
    component:
      css/user/user.theme.css: { every_page: true }

But I don't think we want to do that…

It sure would be nice to avoid having to refactor all this CSS as we move it!

We can't make D8 less shippable.

I didn't mean that this particular issue would do that, but that likely others will have larger repercussions.

isnt the issue here basically that we then have to correct the css in seven so we dont have visual regression ?
then we can move forward ?

Yeah for this particular issue that seems right. !important everything! :P

We talked about this on the Twig call, it's not just a Seven issue. It will be an issue to anyone using Classy as a base theme. We talked about when you declare a CSS dependency it should (within the SMACSS group) alter the source order of the CSS, similar to how the JS dependencies work. We also talked about potentially re-purposing #2474091: CSS files from themes are being loaded in reversed order to discuss this ordering/dependency issue.

All of this should probably be on the parent issue but it's kinda too late. Sorry :(

Another IRC discussion, getting pretty in depth so time to move this higher level discussion to the parent. In the meantime someone could try to implement what I've proposed as a straight hook_library_info_alter(), that would probably be a good POC.

#2489460-20: [Meta] Move module.theme.css files to Classy or Seven

@lauriii #4: All hope is not yet lost :) I've started some work over at #2451411: Add libraries-override to themes' *.info.yml. There's a patch now for testing and review.

On dependencies, AFAICS there is nothing in the codebase that points to ordering by SMACSS groupings, I could be wrong, so this actually needs some discussion.

@cottser do we need a meta where we actually discuss how the css ordering should be ?
- this is gonna be an epic pita further down the road if we dont figure it out ... asap ;)

I think the dependencies ordering deserves discussion but won't solve this.

What we want here is to append to libraries, not make them depend on classy. In their words we want core > classy > seven, not classy > core > seven.

+1 cottser

Going to try and put a patch up here, in the meantime here's a reroll of #21.

Since this one actually needs something like #2497667: Add libraries-extend to themes' *.info.yml and there's not even a patch there yet, here's a patch that should do the trick.

This gets the library definition from classy.libraries.yml and adds it to the 'theme' group in the existing user/drupal.user library, "extending it". Unfortunately getLibraryByName() returns the numeric constant for the group so I think 'theme' needs to be hardcoded.

The screenshot below shows 8.0.x vs. this patch, and that the CSS file is staying in the exact same spot:

Edited to link screenshot.

I can confirm this works as expected. The only thing we need to do it change the file to css/components/user.css

Sure, here's that change.

Great, thanks.

+++ b/core/themes/classy/classy.libraries.yml
@@ -27,3 +27,9 @@ search.results:
+    theme:
+      css/components/user.css: {}

Same question that I've had on other issues. Should this go in a "theme" folder, since it is in the "theme" category.

I think ideally we'll do a follow-up to clean up any inconsistencies if and when these all get in. We're essentially using the theme and component library groups as a way to keep the file loading order the same rather than a meaningful description of what those library elements do. It would be great to straighten that out, but I think it's more important to get the move done first.

We're only going to get one chance at this, unfortunately. We're too close to RC to rely on follow ups.

Classy can have things added to it later, but I don't believe we'll be able to modify what is there come RC and release, so lets make sure we're getting it right when it first goes in.

I agree it should stay in the theme category. I'm suggesting the files go in a theme folder, instead of the components folder where the files that are added in the component category are stored.

+++ b/core/themes/classy/classy.libraries.yml
@@ -27,3 +27,9 @@ search.results:
+    theme:
+      css/components/user.css: {}

This is a component so we need to change this.

That will mess with the loading order, won't it? This might be an exceptional case.

@Cottser It will change the loading order. We've found in other issues like #2553461: Move system icons.theme.css, messages.theme.css, and progress.theme.css to Classy, this should ensure that it's loaded as early as possible.

@LewisNyman So { weight: -10 } ensures the early loading of the file?

The loading order is a combination of category (component, theme, etc) and weight. Also factored in is whether it is added from a module or theme. Files added from modules appear higher. All the "component" files added by themes are somewhat grouped together. The weight will make it get added higher in that grouping.

Great, thank you.

The hook library alter is putting it in the theme group, not component group.

@Cottser Good catch. Here's an updated patch

patch does not apply

The CSS does get moved up but there are no visual regressions, markup diff screenshot and before/after screenshots below. Back to RTBC, thanks!

Before:

After:

Thanks! Setting to postponed for #2566771: [Voltron patch] Move all remaining *.theme.css to Classy