Issue #2566597 by - Cottser, LewisNyman, mdrummond, pjbaert, jaxxed, ManuelGarcia, emma.maria, maked1sky, davidhernandez: [Mega patch] Move system *.theme.css files to Classy

Problem/Motivation

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

We decided to mega patch the system CSS to prevent many many rerolls

Proposed resolution

A patch that includes:

Remaining tasks

Review

Test steps

Check that all CSS file were deleted from the module and added to Classy
Check that the references to that CSS file and library are removed from the module
Check that the CSS file is being loaded in the correct places in Classy
Check that there are no HTML classes in the module that relies on the theme CSS

User interface changes

None for Classy, Stark will be much more Stark

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because nothing is broken
Issue priority Normal but part of a Major meta.
Unfrozen changes Unfrozen because it only changes CSS
Prioritized changes The main goal of this issue is improving themer experience
Disruption Marginally disruptive for themes relying on core CSS, not disruptive for themes using Classy
CommentFileSizeAuthor
#29 Screenshot 2015-09-13 23.29.52.jpg47.01 KBlewisnyman
#29 Screenshot 2015-09-13 23.27.04.jpg477.61 KBlewisnyman
#22 interdiff-19to22.txt3.15 KBdavidhernandez
#22 mega_patch_move-2566597-22.patch18.37 KBdavidhernandez
#19 interdiff-18to19.txt2.44 KBdavidhernandez
#19 mega_patch_move-2566597-19.patch15.68 KBdavidhernandez
#18 interdiff-8to18.txt2.91 KBdavidhernandez
#18 mega_patch_move-2566597-18.patch15.94 KBdavidhernandez
#11 after.png233.67 KBdavidhernandez
#11 before.png182.79 KBdavidhernandez
#8 interdiff-2566597-2-7.txt740 bytesrainbowarray
#8 2566597-7-mega-patch-move-css-classy.patch13.03 KBrainbowarray
#2 mega_patch_move-2566597-2.patch13.03 KBlewisnyman
#2 Screenshot 2015-09-10 23.17.06.jpg1.32 MBlewisnyman

Comments

LewisNyman created an issue. See original summary.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new Screenshot 2015-09-10 23.17.06.jpg1.32 MB
new mega_patch_move-2566597-2.patch13.03 KB

This patch includes all the above issues and the following files:

  • item-list.theme.css
  • field.theme.css
  • tabledrag.theme.css
  • tableselect.theme.css
  • tablesort.theme.css

Here's a screenshot of the loading order:

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented 
+++ b/core/modules/system/templates/menu.html.twig
@@ -27,9 +27,9 @@
-      <ul{{ attributes.addClass('menu') }}>
+      <ul{{ attributes }}>
     {% else %}
-      <ul class="menu">
+      <ul>

Is this suppose to be in here?

This also looks like you are including the clean up stuff to rename the existing libraries? I'm ok with that. Just want to be clear.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented

#3: Yes, that is supposed to be there. In the subissue that involved the menu CSS, we found that the menu template still had the class="menu"; however, all the styles associated with that were purely aesthetic and should be moved to Classy. In tracing things back, the menu class was already moved to Classy. However, it was added back to the core template due to a Toolbar regression. A subsequent issue removed the dependency on the menu class for Toolbar, but the menu class was never removed from core. So it's okay to go, the Classy template already has the menu class.

Status: Needs review » Needs work

The last submitted patch, 2: mega_patch_move-2566597-2.patch, failed testing.

The last submitted patch, 2: mega_patch_move-2566597-2.patch, failed testing.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented
Status: Needs work » Needs review
StatusFileSize
new 2566597-7-mega-patch-move-css-classy.patch13.03 KB
new interdiff-2566597-2-7.txt740 bytes

Took a close look through, comparing this patch to all of the patches on the related issues. Everything looks great. Just reordered one CSS file to match the alphabetical sort order.

I also looked at the failing tests, and I can't for the life of me think what in here would have caused that. We didn't see that error on any of the other issue's tests, so not sure why it would pop up here when we essentially have all the same changes.

Status: Needs review » Needs work

The last submitted patch, 8: 2566597-7-mega-patch-move-css-classy.patch, failed testing.

The last submitted patch, 8: 2566597-7-mega-patch-move-css-classy.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Issue summary: View changes
StatusFileSize
new before.png182.79 KB
new after.png233.67 KB

Here is what I'm seeing. The files all changed from @import to link. Why?

before

after

lewisnyman’s picture

lewisnyman
he/him
commented

Ahhh, so the only reason they are using import is because they go over the number of stylesheets IE can load. I guess that now we have a more even split between module -> base theme -> sub theme we no longer hit that limit.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Oh, ok. We should be able to safely change that test then. The number of files won't grow.

lewisnyman’s picture

lewisnyman
he/him
commented

It might do once we move more and more files into Classy? Is there a way we can remove the assumption completely? We only need to check if the CSS files are being loaded or not. We aren't testing how they are being loaded.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

The test doesn't use Classy. This one in particular installs test_basetheme and test_subtheme and uses them the check the stylesheets related to those themes. So it will only see whatever CSS is loaded by core modules. That is why all the CSS files disappeared.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

I wonder if we can rope #2380651: Remove or rename Classy's layout.css file and adjust tests into this while we're at it. It just involves deleting the layout file and switching the tests to use another one of the files in "base" and remove the stylesheets-remove lines from Seven and Bartik. field.css or form.css are probably good candidates.

lewisnyman’s picture

lewisnyman
he/him
commented

That would be nice.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs work » Needs review
StatusFileSize
new mega_patch_move-2566597-18.patch15.94 KB
new interdiff-8to18.txt2.91 KB

This hopefully fixes those tests.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
StatusFileSize
new mega_patch_move-2566597-19.patch15.68 KB
new interdiff-18to19.txt2.44 KB

Ugh that global isn't needed. It was helping troubleshoot.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

In order to remove the layout file, these tests will have to be changed:

core/modules/system/src/Tests/Installer/InstallerTranslationTest.php
core/modules/system/src/Tests/Theme/ThemeEarlyInitializationTest.php

and remove the stylesheets remove lines from Seven and Bartik:

stylesheets-remove:
  - '@classy/css/layout.css'
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

and of course this from Classy's libraries file.

theme:
css/layout.css: {}

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
StatusFileSize
new mega_patch_move-2566597-22.patch18.37 KB
new interdiff-19to22.txt3.15 KB

This should finally get rid of the empty layout file.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Issue summary: View changes

Yay, green. This is the one. Twig team, assemble!

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented
Status: Needs review » Reviewed & tested by the community

I had previously compared all of the individual move issues to the patch in this issue, and that looked good except for the failing tests. Reviewed the test fixes, those look solid based on the changes in the HTML, and I agree that test is unlikely to break now that we have moved so many CSS files out of core. Also reviewed the changes that lets us remove the unnecessary layout CSS file that was just there for testing: that too looks good.

This looks ready to commit to me.

Here are all the contributors from the individual issues and this one:
- Cottser
- LewisNyman
- mdrummond
- pjbaert
- jaxxed
- ManuelGarcia
- emma.maria
- maked1sky
- davidhernandez

If I missed anybody, sorry.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes

Confirmed, I moved the suggested commit message into the summary:
Issue #2566597 by - Cottser, LewisNyman, mdrummond, pjbaert, jaxxed, ManuelGarcia, emma.maria, maked1sky, davidhernandez: [Mega patch] Move system *.theme.css files to Classy

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Great idea to prevent reroll hell with this. Awesome effort and fantastic work. Committed fb07e7b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 6586221 on 8.0.x 
    Issue #2566597 by Cottser, LewisNyman, mdrummond, pjbaert, jaxxed,...
chx’s picture

chx commented

This was one classy move...

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
StatusFileSize
new Screenshot 2015-09-13 23.27.04.jpg477.61 KB
new Screenshot 2015-09-13 23.29.52.jpg47.01 KB

We have introduced at least one regression into the admin interface.

We took care to make sure the loading order of the CSS stayed the same relative to sub-theme, but something we didn't consider enough was the loading order of module.admin.css files in relation to the module.theme.css (now just Classy CSS) files.

The place block buttons happen to fall into the situation where they are affected by Classy CSS that is now being loaded after when it was being loaded before:

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

loading order of module.admin.css files in relation to the module.theme.css

So there would have been nothing we could do about this since the module CSS will always load first, regardless of category. We'll have to fix the CSS.

Lets open a follow up.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Follow up

#2567845: [regression] Fix any visual regressions from moving the System CSS to Classy

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2601232: Follow-up for #2566597: only attach the 'classy/messages' asset library if there are messages

There was a small oversight here, opened an issue to fix it: #2601232: Follow-up for #2566597: only attach the 'classy/messages' asset library if there are messages.

star-szr’s picture

star-szr
he/him
Primary language English
commented

I'm not sure how yet but I think this somehow broke the color module lock functionality: #2612150: You can no longer see the locks when locking colors together

I did a git bisect that led me here, it seems to work on 9822652 but not 6586221 where this was committed.