Problem/Motivation

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

Proposed resolution

Move the CSS files to classy
Rename the files to remove the .theme. extension
Alter the system.base library so the CSS files are loaded with a similar weight to before

Remaining tasks

Test steps

Check that the CSS file was deleted from the module and added to Classy
Check that the references to that CSS file removed from the system 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 more Stark

API changes

None

CommentFileSizeAuthor
#23 core-move_progress_icons_message_css_to_classy-2553461-23.patch3.81 KBManuel Garcia
#18 2553461.17.css_loads_in_order.png170.37 KBjaxxed
#17 core-move_progress_icons_message_css_to_classy-2553461-17.patch12.93 KBjaxxed
#17 interdiff.15-17.txt797 bytesjaxxed
#15 interdiff.12-15.txt798 bytesjaxxed
#15 core-move_progress_icons_message_css_to_classy-2553461-15.patch12.93 KBjaxxed
#14 Screenshot 2015-09-09 11.44.42.jpg1.49 MBLewisNyman
#14 Screenshot 2015-09-09 11.47.09.jpg35.24 KBLewisNyman
#14 Screenshot 2015-09-09 11.38.19.jpg7.24 KBLewisNyman
#14 Screenshot 2015-09-09 11.38.09.jpg8.16 KBLewisNyman
#12 core-move_progress_icons_message_css_to_classy-2553461-12.patch12.89 KBjaxxed
#6 move_system-2553461-6.patch1.74 KBstar-szr
#4 Screen Shot 2015-08-30 at 1.15.52 pm.png20.49 KBrudraram
#3 move_system-2553461-3.patch1.74 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mdrummond created an issue. See original summary.

mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented

i remember getting heavy pushback on moving icons a bout a year ago - gonna ping wimleers about it, so i can figure out what the issue was

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Active » Needs review
FileSize
move_system-2553461-3.patch1.74 KB

Here's an attempt at messages.theme.css.

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented
FileSize
Screen Shot 2015-08-30 at 1.15.52 pm.png20.49 KB

All the below steps in the test are ok. Attaching a screenshot where the CSS file is being loaded.

2553461-4

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented

@Cottser The patch in #3 is getting rejected.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
FileSize
move_system-2553461-6.patch1.74 KB

Thanks @rudraram, rebased and here is a new patch.

Status: Needs review » Needs work

The last submitted patch, 6: move_system-2553461-6.patch, failed testing.

Status: Needs work » Needs review

rudraram queued 6: move_system-2553461-6.patch for re-testing.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented

This works as expected in Classy, Bartik, and Seven.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria as a volunteer commented
Status: Needs review » Needs work

The issue title and patch work do not match. The patch only contains work for messages.theme.css, are we not doing the rest in this issue? Halp :-)

jaxxed’s picture

jaxxed CreditAttribution: jaxxed at Wunder commented
Assigned: Unassigned » jaxxed
jaxxed’s picture

jaxxed CreditAttribution: jaxxed at Wunder commented
FileSize
core-move_progress_icons_message_css_to_classy-2553461-12.patch12.89 KB

new patch repeats the effort from #6, but duplicates it across progress, messages and icons.

Icons stylesheet is not treated as a library, but rather is a part of the base library. Progress and messages are treated as libraries, and only loaded when the twig templates are loaded.

comments?

jaxxed’s picture

jaxxed CreditAttribution: jaxxed at Wunder commented
Status: Needs work » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
Status: Needs review » Needs work
FileSize
Screenshot 2015-09-09 11.38.09.jpg8.16 KB
Screenshot 2015-09-09 11.38.19.jpg7.24 KB
Screenshot 2015-09-09 11.47.09.jpg35.24 KB
Screenshot 2015-09-09 11.44.42.jpg1.49 MB

This is looking good. I applied the patch and checked to make sure the files were still loading on the pages they should be, including the installation screen.

The only last thing concerning me is that the Classy files are loading after the sub-theme, meaning that they would overwrite any sub-theme CSS that conflicts. Is there an issue about this general problem with base themes?

In #2549563: Move system action-links.theme.css, breadcrumb.theme.css, and button.theme.css to Classy, it looks like they've solved the problem by keeping the weight: -10 value for the file. I think we could also do that here.

jaxxed’s picture

jaxxed CreditAttribution: jaxxed at Wunder commented
Status: Needs work » Needs review
FileSize
core-move_progress_icons_message_css_to_classy-2553461-15.patch12.93 KB
interdiff.12-15.txt798 bytes

Updated #12 patch to lower weights

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work

Sorry! Turns out that wasn't the solution:

+++ b/core/themes/classy/classy.libraries.yml
@@ -16,12 +17,24 @@ drupal.comment.threaded:
+    theme:
+      css/components/progress.css: { weight: -10 }

We need to change 'theme' to 'component'

jaxxed’s picture

jaxxed CreditAttribution: jaxxed at Wunder commented
FileSize
interdiff.15-17.txt797 bytes
core-move_progress_icons_message_css_to_classy-2553461-17.patch12.93 KB

Latest re-roll, changes theme elements to components.

jaxxed’s picture

jaxxed CreditAttribution: jaxxed at Wunder commented
FileSize
2553461.17.css_loads_in_order.png170.37 KB

css order on latest patch (please confirm)

jaxxed’s picture

jaxxed CreditAttribution: jaxxed at Wunder commented
Status: Needs work » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Assigned: jaxxed » Unassigned
Status: Needs review » Reviewed & tested by the community

I can confirm that the CSS loading order is now correct. Thank you! *RTBC wand*

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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?

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia at Appnovation commented
Assigned: Unassigned » Manuel Garcia

rrrrrrrrrerollin'

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia at Appnovation commented
Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
core-move_progress_icons_message_css_to_classy-2553461-23.patch3.81 KB

Reroll of #17. Fixed conflicts in:

  • core/modules/system/system.libraries.yml
  • core/themes/classy/classy.libraries.yml
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Thanks! Looking good.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Reviewed & tested by the community » Postponed

Postponed on: #2566597: [Mega patch] Move system *.theme.css files to Classy

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented
Status: Postponed » Closed (fixed)

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