Problem/Motivation

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

Proposed resolution

Move the CSS file to classy
Rename the file to remove the .theme. extension
Alter the system.base library so the CSS file is 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

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because coding standards
Issue priority Not critical because coding standards
Unfrozen changes Unfrozen because it only changes CSS
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mdrummond created an issue. See original summary.

RainbowArray’s picture

Issue summary: View changes
davidhernandez’s picture

Status: Active » Needs review
FileSize
4.54 KB

Status: Needs review » Needs work

The last submitted patch, 3: move_system-2553449-3.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: move_system-2553449-3.patch, failed testing.

maked1sky’s picture

FileSize
13.97 KB

This is re-rolled patch

star-szr’s picture

@maked1sky thanks, I think if you update your git configuration to add the following it will generate a smaller patch:

[diff]
  renames = copies

See https://www.drupal.org/documentation/git/configure

However I think we can simplify this patch.

+++ b/core/modules/system/system.libraries.yml
@@ -27,7 +27,6 @@ base:
-      css/components/field.theme.css: { every_page: true, weight: -10 }

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

+++ b/core/themes/classy/templates/field/field--comment.html.twig
@@ -44,6 +44,7 @@
+{{ attach_library('classy/field') }}

If it was every_page before, why change that and add all these attach_library? Just keep it simple, every_page in classy.libraries.yml! :)

star-szr’s picture

For the every_page part see also Lewis' comment in #2553465-4: Move system exposed-filters.theme.css, more-link.theme.css, and pager.theme.css to Classy, he adds some important info (library needs to be added to .info.yml as well).

davidhernandez’s picture

every_page doesn't exist any more. It was removed from core. Make sure your local repos are up to date. I think it was removed last week.

star-szr’s picture

Regardless of the every_page bit I think we can still attach this via the .info.yml.

RainbowArray’s picture

Reroll. There was some weirdness with the field template. Please double-check.

RainbowArray’s picture

Status: Needs work » Needs review
RainbowArray’s picture

Status: Needs review » Needs work

The last submitted patch, 14: 2553449-13-move-field-css-classy.patch, failed testing.

The last submitted patch, 14: 2553449-13-move-field-css-classy.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

We had a bad testbot I'm fairly sure :)

Status: Needs review » Needs work

The last submitted patch, 14: 2553449-13-move-field-css-classy.patch, failed testing.

RainbowArray’s picture

Trying this on the base library rather than creating a new global-styling library to see if that resolves some of the errors.

Status: Needs review » Needs work

The last submitted patch, 21: 2553449-21-move-field-css-classy.patch, failed testing.

RainbowArray’s picture

No need to keep resubmitting these tests. I remember the tests in question from the field markup cleanup, and they're nasty little beasties, because they're entirely dependent on the spacing in the source output. It would be really nice if we could refactor those tests so they still test what they're supposed to test without being so picky.

The last submitted patch, 21: 2553449-21-move-field-css-classy.patch, failed testing.

star-szr’s picture

Maybe a silly question but why are we changing markup here? If we revert the change to the template will the tests pass?

emma.maria’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
456 bytes

I reverted the changes to the template as per the suggestion in #26.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
842.54 KB
835.69 KB

Here are some screenshots from Classy and Bartik to show that they look as they did before. Note that Bartik actually overrides the inline label CSS for tags. Also that float problem with the comments form already exists.

davidhernandez’s picture

This one looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs reroll.

LewisNyman’s picture

Status: Needs work » Postponed
davidhernandez’s picture

Status: Postponed » Closed (fixed)

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