Suggested commit credit:

Issue #2395853 by Manjit.Singh, LewisNyman, nlisgo, pjbaert, Branislav Bujisic, pguillard, gavin.hughes, alisonjo2786, onelittlebecca, camoa, DickJohnson, manmohandream, davidhernandez, kgoel: Split system.module.css and system.theme.css files into SMACSS style components

Problem/Motivation

The final phase of #1921610: [Meta] Architect our CSS is to rewrite the CSS selectors in core to match our CSS standards. See #1995272: [Meta] Refactor module CSS files inline with our CSS standards

We've been splitting up the CSS files of the themes in Bartik and Seven to make it easier to optimise each one and reduce the scope of the changes to one CSS file.

#2375673: Split Bartik's CSS into SMACSS style components
#2321505: Split Seven's style.css into SMACSS categories

As system contains so many CSS components, it makes sense to split this in the same way, so file names can be consistent across core.

Proposed resolution

Split system.theme.css and system.module.css into smaller files.

Remaining tasks

  • Figure out why automated tests fail if there are more than 23 css files in base css section of system.libraries.yml
  • Continue splitting rest of css files: system.diff.css, system.maintenance.css, system.module.css and system.theme.css

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because CSS standards
Unfrozen changes Unfrozen because it only changes CSS
Disruption This only affects CSS files. There should be little or no disruption.
CommentFileSizeAuthor
#118 interdiff-2395853-108-118.txt1.26 KBnlisgo
#118 split_system_module_css-2395853-118.patch62.51 KBnlisgo
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,322 pass(es). View
#108 split_system_module_css-2395853-108.patch63.28 KBLewisNyman
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,925 pass(es). View
#108 interdiff.txt435 bytesLewisNyman
#105 split_system_module_css-2395853-105.patch63.29 KBLewisNyman
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,960 pass(es). View
#96 interdiff.txt1.59 KBalisonjo2786
#96 2395853-96.patch62.88 KBalisonjo2786
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2395853-96.patch. Unable to apply patch. See the log in the details link for more information. View
#91 interdiff.txt5.17 KBonelittlebecca
#91 2395853-91.patch62.88 KBonelittlebecca
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,835 pass(es). View
#88 split_system_module_css-2395853-88.patch62.98 KBLewisNyman
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,267 pass(es). View
#88 interdiff.txt15.61 KBLewisNyman
#78 split_system_module_css-2395853-78.patch63.59 KBLewisNyman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,834 pass(es). View
#78 interdiff.txt16.72 KBLewisNyman
#76 system-smacss-backstop-3.pdf24.44 MBLewisNyman
#75 system-smacss-backstop-2.pdf3.19 MBLewisNyman
#75 system-smacss-backstop-1.pdf3.26 MBLewisNyman
#75 interdiff.txt9.82 KBLewisNyman
#75 split_system_module_css-2395853-71.patch68.52 KBLewisNyman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,818 pass(es). View
#71 split_system_module_css-2395853-71.patch68.52 KBgavin.hughes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,567 pass(es). View
#67 split_system_module_css-2395853-66.patch67.55 KBgavin.hughes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,726 pass(es). View
#63 split_system_module_css-2395853-63.patch66.85 KBManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,654 pass(es). View
#59 split_system_module_css-2395853-59.patch55.05 KBmanmohandream
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch split_system_module_css-2395853-59.patch. Unable to apply patch. See the log in the details link for more information. View
#57 split_system_module_css-2395853-57.patch66.19 KBpjbaert
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,287 pass(es). View
#57 interdiff-2395853-51-57.txt22 KBpjbaert
#51 split_system_module_css-2395853-51.patch46.07 KBManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,289 pass(es). View
#48 interdiff-2395853-45-48.txt5.98 KBManjit.Singh
#48 split_system_module_css-2395853-48.patch46.07 KBManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,268 pass(es). View
#45 split_system_module_css-2395853-45.patch47.33 KBpjbaert
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,171 pass(es). View
#45 interdiff-2395853-39-45.txt17.12 KBpjbaert
#39 interdiff.txt77.33 KBcamoa
#39 split_system_module_css-2395853-39.patch47.51 KBcamoa
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,412 pass(es). View
#35 interdiff-2395853-32-35.txt8.86 KBManjit.Singh
#35 split_system_css_files-2395853-35.patch57.74 KBManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,771 pass(es). View
#34 2395853-BackstopJS Report.pdf3.18 MBLewisNyman
#32 interdiff.txt3.92 KBpguillard
#32 split_system_css_files-2395853-32.patch49.69 KBpguillard
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,681 pass(es). View
#28 split_system_css_files-2395853-28.patch50.57 KBpguillard
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch split_system_css_files-2395853-28.patch. Unable to apply patch. See the log in the details link for more information. View
#28 interdiff.txt1.96 KBpguillard
#25 split_system_css_files-2395853-25.patch50.56 KBbbujisic
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch split_system_css_files-2395853-25.patch. Unable to apply patch. See the log in the details link for more information. View
#25 interdiff-2395853-18-25.txt5.53 KBbbujisic
#18 split_system_css_files-2395853-18.patch50.59 KBnlisgo
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,481 pass(es). View
#18 interdiff-2395853-15-18.txt790 bytesnlisgo
#15 split_system_css_files-2395853-15.patch49.82 KBnlisgo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,433 pass(es), 1 fail(s), and 0 exception(s). View
#15 interdiff-2395853-10-15.txt3.74 KBnlisgo
#10 interdiff-2395853-2-10.txt2.72 KBbbujisic
#10 smacss-system-module-css-2395853-10.patch46.48 KBbbujisic
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,448 pass(es), 7 fail(s), and 0 exception(s). View
#2 smacss-system-module-css-2395853-2.patch52.54 KBDickJohnson
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,215 pass(es), 7 fail(s), and 0 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

DickJohnson’s picture

Ok, let's take a look what we have here:

system.module.css

  • Animated throbber, lines 12-27
  • Fieldgroup reset, rows 29-35
  • Collapse.js styles, rows 37-44
  • Resizable textareas, rows 46-70
  • Tabledrag.js styles, rows 72-167
  • Tableheader.js styles, rows 169-179
  • Progress.js styles, rows 181-228
  • Throbber, rows 230-270
  • Container inline, rows 272-282
  • Form inline, rows 283-307
  • Various reusable classes, rows 309-448

Last one is probably the most difficult from SMACSS point of view.

system.admin.css

  • Layout styles, rows 6-45
  • Admin panel, rows 48-58 #2399939: Refactor 'admin-panel' CSS component
  • Compact links, rows 59-63
  • Inline admin links, rows 66-74
  • Modules page styles, rows 76-188
  • Status report styles, rows 190-222
  • Appearance page, rows 224-267
  • Vertical toolbar, rows 269-374
  • Exposed filters, rows 376-419

system.theme.css

  • Node unpublished, rows 6-11
  • Tablesort indicator, rows 13-21
  • Item list, rows 23-39
  • Various styles for things generated by Form API, rows 41-114
  • Container inline, rows 116-133
  • More link, rows 135-144
  • More help, rows 146-162
  • Pager item(s), rows 164-179
  • Button link, rows 181-195
  • Collapse.js styles, rows 197-244
  • TableDrag.js styles, rows 246-259
  • TableSelect.js styles, rows 261-272
  • Progress bar, rows 274-335
  • Menu styles, rows 337-368
  • Links, rows 370-385
  • Breadcrumbs, rows 387-413
  • Tabs, rows 415-445
  • Action links, rows 447-487
  • Messages, rows 489-561
  • Fields, rows 563-601
DickJohnson’s picture

Status: Active » Needs review
FileSize
52.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,215 pass(es), 7 fail(s), and 0 exception(s). View

Did a minor patch on this also.

Created following files to css/components directory, and copies styles related to these from system.module.css, system.theme.css and system.admin.css:

action-links.css
admin-panel.css
animated-throbber.css
appearance-page.css
breadcrumbs.css
button.css
compact-links.css
container-inline.css
exposed-filters.css
field.css
fieldgroup.css
form.css
inline-admin-links.css
inline-form.css
item-list.css
links.css
menu.css
messages.css
module-page-styles.css
more-help.css
more-link.css
node-unpublished.css
pager.css
progress.css
status-report.css
table-select.css
tabledrag.css
tableheader.css
tablesort.css
tabs.css
textarea.css
throbber.css
vertical-toolbar.css

Created following file to css/layout.css and copied styles related to this from system.admin.css.

I didn't touch the reusable components on system.module.css or form api related things on system.theme.css.

There probably will be issues in things like tabledrag.js which comes originally from two different files.

Sending to testbot.

idebr’s picture

How does this integrate with the module/theme file naming conventions? Eg. are we using button.module.css and button.theme.css or just button.css?

Status: Needs review » Needs work

The last submitted patch, 2: smacss-system-module-css-2395853-2.patch, failed testing.

DickJohnson’s picture

Assigned: Unassigned » DickJohnson

I think we're using button.module.css and button.theme.css. I'll go there within next few days, five'ish hours with this was enough for one day.

LewisNyman’s picture

hmm, I can't find any documentation on component styling for modules in the standards. I think we should still maintain the .module.css and .theme.css split.

LewisNyman’s picture

Assigned: DickJohnson » Unassigned
bbujisic’s picture

I'm going to look at it.

bbujisic’s picture

Issue tags: +Needs tests

Added Needs tests tag because tests need to be updated.

bbujisic’s picture

Status: Needs work » Needs review
FileSize
46.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,448 pass(es), 7 fail(s), and 0 exception(s). View
2.72 KB

Attached patch fixes several typos in system.libraries.yml and separates administrative css files (ones originated from system.admin.css file) into admin section of yml file.

Status: Needs review » Needs work

The last submitted patch, 10: smacss-system-module-css-2395853-10.patch, failed testing.

bbujisic’s picture

Issue summary: View changes

For some reason, automated testing fails whenever there are more than 23 css files in base css section of system.libraries.yml file. Could anyone check what's wrong with tests?

I manually tested HTML file generated by Drupal\system\Tests\Theme\ThemeInfoTest test, and it was supposed to pass.

LewisNyman’s picture

nlisgo’s picture

Assigned: Unassigned » nlisgo

I'm going to see if I can work through this issue.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
49.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,433 pass(es), 1 fail(s), and 0 exception(s). View

I believe I have addressed the errors with Drupal\system\Tests\Theme\ThemeInfoTest::testStylesheets but not yet with Drupal\search\Tests\SearchQueryAlterTest::testQueryAlter.

The issue with Drupal\system\Tests\Theme\ThemeInfoTest::testStylesheets is because the tests were looking for css to be included using the <link rel="stylesheet" href="" /> element but because this path pushes the number of css included files to over 31 the way that css files are included switches to using @import url().

I have refactored the tests in Drupal\system\Tests\Theme\ThemeInfoTest::testStylesheets accordingly.

I am not sure at all why Drupal\search\Tests\SearchQueryAlterTest::testQueryAlter now. I have checked the html output generated in testing when setting the verbose flag and I cannot see the word 'page' on the page.

Needs to be investigated further. So I expect this test to produce that 1 fail.

I would appreciate feedback on the refactor of the tests in Drupal\system\Tests\Theme\ThemeInfoTest::testStylesheets.

nlisgo’s picture

Assigned: nlisgo » Unassigned

Status: Needs review » Needs work

The last submitted patch, 15: split_system_css_files-2395853-15.patch, failed testing.

nlisgo’s picture

Status: Needs work » Needs review
Issue tags: +drupaldevdays
FileSize
790 bytes
50.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,481 pass(es). View

The reason why Drupal\search\Tests\SearchQueryAlterTest::testQueryAlter is failing is because the method Drupal\simpletest\AssertContentTrait::getTextContent is also returning the text within the <style> element in the head. Because there is a css file with the word page in it the text is found and the test fails.

I have refactored the test so it will avoid this issue while not invalidating the test.

nlisgo’s picture

Issue summary: View changes
Issue tags: -Needs tests

Can someone verify that this task still needs to be done:

Continue splitting rest of css files: system.diff.css, system.maintenance.css, system.module.css and system.theme.css

nlisgo’s picture

Since the remaining task has been added in #12 I am marking this as needs work.

To be clear, the remaining task is:

Continue splitting rest of css files: system.diff.css, system.maintenance.css, system.module.css and system.theme.css

I believe that this was put on hold while the tests were failing but that has been cleared now.

@branislav-bujisic, are you available to look at this again?

nlisgo’s picture

Status: Needs review » Needs work
LewisNyman’s picture

Title: Split system CSS files into SMACSS style components » Split system.module.css and system.theme.css files into SMACSS style components
Status: Needs work » Needs review
Issue tags: +Needs visual regression testing

I think that we have already bitten off a lot in one patch. The more CSS we change in one patch the harder it becomes to test. We need some visual regression testing to verify that we haven't broken anything.

I think quite a lot of things will break before #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name is committed.

LewisNyman’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/system.libraries.yml
    @@ -13,7 +40,15 @@ admin:
    +      css/layout.css: {}
    

    This layout file isn't loading because Seven has this file already. I think for now we should rename it to layout.admin.css?

  2. +++ b/core/modules/system/system.libraries.yml
    @@ -13,7 +40,15 @@ admin:
    +      css/components/module-page-styles.css: {}
    

    I think this file can just be called module page

lauriii’s picture

bbujisic’s picture

Status: Needs work » Needs review
FileSize
5.53 KB
50.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch split_system_css_files-2395853-25.patch. Unable to apply patch. See the log in the details link for more information. View

In light of #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name, would it be reasonable to keep layout.css as-is and rename the module-page-styles.css file to modules-page.css (which already exists in Seven theme)?

Status: Needs review » Needs work

The last submitted patch, 25: split_system_css_files-2395853-25.patch, failed testing.

LewisNyman’s picture

pguillard’s picture

FileSize
1.96 KB
50.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch split_system_css_files-2395853-28.patch. Unable to apply patch. See the log in the details link for more information. View

Oops, I was rerolling it at the same time.
I post it there anyway, just in case...

LewisNyman’s picture

@pguillard That's ok, it will hopefully still apply after the other is committed

LewisNyman’s picture

Status: Needs review » Needs work

The last submitted patch, 28: split_system_css_files-2395853-28.patch, failed testing.

pguillard’s picture

FileSize
49.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,681 pass(es). View
3.92 KB

Rerolled again

nlisgo’s picture

Status: Needs work » Needs review

Queuing the patch in #32 for testing.

LewisNyman’s picture

Status: Needs review » Needs work
FileSize
3.18 MB

Ok, so I ran some visual regression tests and I didn't get any regressions! It seems like we still to move some more stuff out of the system css files and into separate files, so setting to needs work.

Manjit.Singh’s picture

FileSize
57.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,771 pass(es). View
8.86 KB

removed some css from system.theme.css, because We have move that in components with respective to their directories. Uploading interdiff for ease :)

Manjit.Singh’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work

Nice, thanks! I went over the file names to make sure that they match the name of the component class. This makes it easy for a frontend developer to easily find the name of the file after looking at the HTML class in a browser inspector. The exceptions are modules-page and appearance-page which are not yet componentised.

  1. +++ b/core/modules/system/css/components/container-inline.css
    @@ -0,0 +1,33 @@
    + * @file
    + * Styles for inlien items. Rows 7-14 from system.module.css.
    + * Rows 16-33 from system.theme.css
    + */
    +
    +.container-inline div,
    +.container-inline label {
    +  display: inline;
    +}
    +/* Details contents always need to be rendered as block. */
    +.container-inline .details-wrapper {
    +  display: block;
    +}
    +
    +/**
    + * Inline items.
    + */
    +.container-inline label:after,
    +.container-inline .label:after {
    

    Can we remove the top comment and merge this file together?

  2. +++ b/core/modules/system/css/components/progress.css
    @@ -0,0 +1,115 @@
    + * @file
    + * Visual styles for progress bar.
    + *
    + * from system.theme.css
    ...
    + * Progress behavior.
    + *
    + * from system.module.css
    

    Can we remove these comments that reference the original files?

  3. +++ /dev/null
    @@ -1,370 +0,0 @@
    -/**
    - * Panel.
    - * Used to visually group items together.
    - */
    -.panel {
    -  padding: 5px 5px 15px;
    -}
    -.panel__description {
    -  margin: 0 0 3px;
    -  padding: 2px 0 3px 0;
    -}
    -
    

    We need to move this CSS into panel.css and delete admin-panel.css

  4. +++ /dev/null
    @@ -1,370 +0,0 @@
    -/**
    - * System compact link: to toggle the display of description text.
    - */
    -.compact-link {
    -  margin: 0 0 0.5em 0;
    -}
    

    The name of the corresponding file should be 'compact-link' instead of 'compact-links'

  5. +++ /dev/null
    @@ -1,370 +0,0 @@
    -/**
    - * Quick inline admin links.
    - */
    -small .admin-link:before {
    -  content: ' [';
    -}
    -small .admin-link:after {
    -  content: ']';
    

    The name of this file should be 'admin-link'

  6. +++ b/core/modules/system/css/system.module.css
    @@ -4,37 +4,6 @@
    - * Autocomplete.
    - *
    - * @see autocomplete.js
    - */
    -
    -/* Animated throbber */
    -.js input.form-autocomplete {
    

    Can we call this file form-autocomplete?

  7. +++ b/core/modules/system/css/system.theme.css
    @@ -154,37 +106,6 @@ abbr.ajax-changed {
    -/**
    - * Show buttons as links.
    - */
    -button.link {
    -  background: transparent;
    

    The name of this file should be 'link.css' instead of 'button.css'

  8. +++ b/core/modules/system/css/system.theme.css
    @@ -232,361 +153,3 @@ summary {
    -/**
    - * Markup generated by breadcrumb.html.twig.
    - */
    -.breadcrumb {
    

    This file should be called 'breadcrumb.css' instead of 'breadcrumbs.css'

  9. +++ b/core/modules/system/css/components/textarea.css
    --- /dev/null
    +++ b/core/modules/system/css/components/throbber.css
    
    +++ b/core/modules/system/css/components/throbber.css
    +++ b/core/modules/system/css/components/throbber.css
    @@ -0,0 +1,46 @@
    
    @@ -0,0 +1,46 @@
    +/**
    + * @file
    + * Visual styles for throbbers.
    + */
    +
    +.ajax-progress {
    

    Can we call this file ajax-progress?

+++ b/core/modules/system/css/components/links.css
@@ -13,6 +13,6 @@ ul.inline li {
 }
-ul.links a.active {
+ul.links a.is-active {
   color: #000;

Can we avoid making improvements and fixes in this issue? It becomes harder to track the movement of the code if it changes.

LewisNyman’s picture

We had a discussion about how to structure the CSS files across the modules and Classy, see: #2489460: [Meta] Move module.theme.css files to Classy or Seven

In this issue we need to maintain the seperation between the system.module.css file and the system.theme.css file so we can move the theme style to Classy later.

The naming conventions should be: [component].module.css and [component].theme.css

camoa’s picture

FileSize
47.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,412 pass(es). View
77.33 KB

Based on the conversation had on DrupalCon Los Angeles we need to separate system and components back to M.A.T so we can move the theme CSS.

This patch takes things one step back so we can start working from here on doing so. We still want components, I understand, but we want to have identified the CSS as:
- component.module.css
- component.theme.css
- component.admin.css

I am just doing that, bringing system.admin.css back and refactoring the files for components to identify what is theme and what is module.

We can then retake the work of separating components using this three files so we can eventually move all non functional or admin CSS to classy.

LewisNyman’s picture

Status: Needs work » Needs review
LewisNyman’s picture

LewisNyman’s picture

Status: Needs review » Needs work

Thanks for this!

+++ b/core/modules/system/css/system.admin.css
@@ -9,6 +9,7 @@
+
 .layout-container:after {
   content: "";
   display: table;

@@ -19,26 +20,33 @@
+
   .layout-column {
-    float: left;  /* LTR */
+    float: left; /* LTR */

This patch makes a few spacing changes to system.admin.css - we should avoid making these kinds of changes in this issue. Setting to needs work.

There is still a lot of CSS in system.module.css and system.theme.css. I'll make some suggestions for the remaining CSS in system.theme.css:

.node--unpublished {
  background-color: #fff4f4;
}

This CSS is already in node-unpublished.theme.css but it should be called node.theme.css, no idea why this is in system instead of node but let's move/delete it in another issue.

th.is-active img {
  display: inline;
}
td.is-active {
  background-color: #ddd;
}

This is already moved into tablesort.css so we can remove this

.form-item,
.form-actions {
  margin-top: 1em;
  margin-bottom: 1em;
}

All the form stuff should go into form.theme.css

.icon-help {
  background: url(../../../misc/help.png) 0 50% no-repeat; /* LTR */
  padding: 1px 0 1px 20px; /* LTR */
}

This should be in icons.theme.css

details {
  border: 1px solid #ccc;
  margin-top: 1em;
  margin-bottom: 1em;
}
details > .details-wrapper {
  padding: 0.5em 1.5em;
}
/* @todo Regression: The summary of uncollapsible details are no longer
     vertically aligned with the .details-wrapper in browsers without native
     details support. */
summary {
  cursor: pointer;
  padding: 0.2em 0.5em;
}

We shouldn't be styling default elements in modules... but for now let's put this in details.theme.css

.collapse-processed > summary {
  padding-left: 0.5em;
  padding-right: 0.5em;
}
.collapse-processed > summary:before {
  background: url(../../../misc/menu-expanded.png) 0px 100% no-repeat; /* LTR */
  content: "";
  float: left;
  height: 1em;
  width: 1em;
}

collapse-processed.theme.css

LewisNyman’s picture

Priority: Normal » Major

Bumping this to major because many issues rely on this before they can make improvements to this CSS. It also affects a lot of CSS that's loaded on every page!

pjbaert’s picture

Assigned: Unassigned » pjbaert
pjbaert’s picture

Status: Needs work » Needs review
FileSize
17.12 KB
47.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,171 pass(es). View

I processed the #42 remarks & suggestions by @LewisNyman in this patch

pjbaert’s picture

Assigned: pjbaert » Unassigned
LewisNyman’s picture

Status: Needs review » Needs work

Thanks, there are now only two groups of CSS left in system.theme.css:

/**
 * Publishing status.
 */
.node--unpublished {
  background-color: #fff4f4;
}

We can delete this, it's already been moved.

.button,
.image-button {
  margin-left: 1em;
  margin-right: 1em;
}
.button:first-child,
.image-button:first-child {
  margin-left: 0;
  margin-right: 0;
}

This code can go in button.theme.css

Also:

/**
 * @file
 * Visual styles for buttons.
 */
button.link {
  background: transparent;
  border: 0;
  cursor: pointer;
  margin: 0;
  padding: 0;
  font-size: 1em;
}
label button.link {
  font-weight: bold;
}

The class here is .link so this code should be in link.theme.css instead of button.theme.css

  1. +++ b/core/modules/system/css/components/button.theme.css
    --- /dev/null
    +++ b/core/modules/system/css/components/collapse-processed.theme.css
    
    +++ b/core/modules/system/css/components/collapse-processed.theme.css
    +++ b/core/modules/system/css/components/collapse-processed.theme.css
    @@ -0,0 +1,27 @@
    
    @@ -0,0 +1,27 @@
    +.collapse-processed > summary {
    

    This file needs a @file comment

  2. +++ b/core/modules/system/css/components/collapse-processed.theme.css
    --- /dev/null
    +++ b/core/modules/system/css/components/container-inline.theme.css
    
    +++ b/core/modules/system/css/components/container-inline.theme.css
    +++ b/core/modules/system/css/components/container-inline.theme.css
    @@ -0,0 +1,33 @@
    
    @@ -0,0 +1,33 @@
    +/**
    + * @file
    + * Styles for inlien items. Rows 7-14 from system.module.css.
    + * Rows 16-33 from system.theme.css
    

    There is a typo here, also can we remove the information about the rows?

  3. +++ b/core/modules/system/css/components/details.theme.css
    @@ -0,0 +1,21 @@
    + * Collapsible details.
    + *
    + * @see collapse.js
    + * @thanks http://nicolasgallagher.com/css-background-image-hacks/
    + */
    +details {
    

    There should be a blank line after the @file comment block

  4. +++ b/core/modules/system/css/components/details.theme.css
    --- /dev/null
    +++ b/core/modules/system/css/components/exposed-filters.theme.css
    
    +++ b/core/modules/system/css/components/exposed-filters.theme.css
    +++ b/core/modules/system/css/components/exposed-filters.theme.css
    @@ -0,0 +1,45 @@
    
    @@ -0,0 +1,45 @@
    +/**
    + * @file
    + * Visual styles for exposed filters.
    + */
    +.exposed-filters .filters {
    

    There should be a blank line after the @file comment block

  5. +++ b/core/modules/system/css/components/form.theme.css
    --- /dev/null
    +++ b/core/modules/system/css/components/icons.theme.css
    
    +++ b/core/modules/system/css/components/icons.theme.css
    +++ b/core/modules/system/css/components/icons.theme.css
    @@ -0,0 +1,9 @@
    
    @@ -0,0 +1,9 @@
    +/* Style for the help icon. */
    +.icon-help {
    

    This needs to be a proper @file comment block

  6. +++ b/core/modules/system/css/components/table-select.theme.css
    --- /dev/null
    +++ b/core/modules/system/css/components/tabledrag.theme.css
    
    +++ b/core/modules/system/css/components/tabledrag.theme.css
    +++ b/core/modules/system/css/components/tabledrag.theme.css
    @@ -0,0 +1,117 @@
    
    @@ -0,0 +1,117 @@
    +/**
    + * @file
    + * Visual styles for table drag
    

    This comment needs a full stop

  7. +++ b/core/modules/system/css/components/throbber.theme.css
    --- a/core/modules/system/css/system.admin.css
    +++ b/core/modules/system/css/system.admin.css
    
    +++ b/core/modules/system/css/system.admin.css
    +++ b/core/modules/system/css/system.admin.css
    @@ -14,13 +14,12 @@
    
    @@ -14,13 +14,12 @@
       display: table;
       clear: both;
     }
    -
    
    @@ -55,6 +54,14 @@
    +.admin-panel {
    +  margin: 0;
    +  padding: 5px 5px 15px 5px;
    +}
    +.admin-panel .description {
    +  margin: 0 0 3px;
    +  padding: 2px 0 3px 0;
    +}
    

    Can we revert all changes to system.admin.css? So we don't touch it at all in this patch.

Manjit.Singh’s picture

FileSize
46.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,268 pass(es). View
5.98 KB

#47 Changes, suggested by lewis are done.. Also revert the changes of system.admin.css file.

Manjit.Singh’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/css/components/button.theme.css
    @@ -0,0 +1,14 @@
    + * @file
    + * Visual styles for buttons.
    + */
    +.button,
    

    This @file comment news a blank line

  2. +++ b/core/modules/system/css/components/item-list.theme.css
    --- /dev/null
    +++ b/core/modules/system/css/components/links.theme.css
    
    +++ b/core/modules/system/css/components/links.theme.css
    @@ -0,0 +1,29 @@
    +ul.links a.is-active {
    +  color: #000;
    +}
    +button.link {
    +  background: transparent;
    

    I think that it's better to put the button.link stuff into link.theme.css - because the two classes are not related even though they sound it

  3. +++ b/core/modules/system/system.libraries.yml
    @@ -4,8 +4,37 @@ base:
           css/system.theme.css: { every_page: true, weight: -10 }
    

    We need to remove this this file from the yml file. Also, I guess we should also apply those every_page and weight settings to all of the files?

Manjit.Singh’s picture

Status: Needs work » Needs review
FileSize
46.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,289 pass(es). View

Changes suggested in #50

+++ b/core/modules/system/css/components/links.theme.css
@@ -0,0 +1,29 @@
+}
+button.link {
+  background: transparent;
+  border: 0;
+  cursor: pointer;
+  margin: 0;
+  padding: 0;
+  font-size: 1em;
+}

@lewis This is already in links.theme.css or am i missing something.

LewisNyman’s picture

@lewis This is already in links.theme.css or am i missing something.

Yep :) I think that we should put it in link.theme.css

Thanks for working on this! I'll also go through the system.module.css file and make suggestions. There are a lot of utility classes in there that would be really tedious to have in individual files.

LewisNyman’s picture

Status: Needs review » Needs work

In system.module.css:

/**
 * Collapsible details.
 *
 * @see collapse.js
 */
.js details:not([open]) .details-wrapper {
  display: none;
}

details.module.css

/**
* Resizable textareas.
*/
.form-textarea-wrapper textarea {
display: block;
margin: 0;
width: 100%;
box-sizing: border-box;
}
.resize-none {
resize: none;
}
.resize-vertical {
resize: vertical;
min-height: 2em;
}
.resize-horizontal {
resize: horizontal;
max-width: 100%;
}
.resize-both {
resize: both;
max-width: 100%;
min-height: 2em;
}

resize.module.css

/**
 * TableDrag behavior.
 *
 * @see tabledrag.js
 */
body.drag {
  cursor: move;
}
tr.region-title {
  font-weight: bold;
}
tr.region-message {
  color: #999;
}
tr.region-populated {
  display: none;
}
tr.add-new .tabledrag-changed {
  display: none;
}
.draggable a.tabledrag-handle {
  cursor: move;
  float: left; /* LTR */
  height: 1.7em;
  margin-left: -1em; /* LTR */
  overflow: hidden;
  text-decoration: none;
}
[dir="rtl"] .draggable a.tabledrag-handle {
  float: right;
  margin-right: -1em;
  margin-left: 0;
}
a.tabledrag-handle:hover {
  text-decoration: none;
}
a.tabledrag-handle .handle {
  background: url(../../../misc/icons/787878/move.svg) no-repeat 6px 7px;
  height: 14px;
  margin: -0.4em 0.5em 0;
  padding: 0.42em 0.5em;
  width: 14px;
}
a.tabledrag-handle:hover .handle,
a.tabledrag-handle:focus .handle {
  background-image: url(../../../misc/icons/000000/move.svg);
}
.touch .draggable td {
  padding: 0 10px;
}
.touch .draggable .menu-item__link {
  display: inline-block;
  padding: 10px 0;
}
.touch a.tabledrag-handle {
  height: 44px;
  width: 40px;
}
.touch a.tabledrag-handle .handle {
  background-position: 40% 19px;
  height: 21px;
}
.touch .draggable.drag a.tabledrag-handle .handle {
  background-position: 50% -32px;
}
div.tree-child {
  background: url(../../../misc/tree.png) no-repeat 11px center; /* LTR */
}
div.tree-child-last {
  background: url(../../../misc/tree-bottom.png) no-repeat 11px center; /* LTR */
}
[dir="rtl"] div.tree-child,
[dir="rtl"] div.tree-child-last {
  background-position: -65px center;
}
div.tree-child-horizontal {
  background: url(../../../misc/tree.png) no-repeat -11px center;
}
.tabledrag-toggle-weight-wrapper {
  text-align: right; /* LTR */
}
[dir="rtl"] .tabledrag-toggle-weight-wrapper {
  text
}
.indentation {
  float: left; /* LTR */
  height: 1.7em;
  margin: -0.4em 0.2em -0.4em -0.4em; /* LTR */
  padding: 0.42em 0 0.42em 0.6em; /* LTR */
  width: 20px;
}
[dir="rtl"] .indentation {
  float: right;
  margin: -0.4em -0.4em -0.4em 0.2em;
  padding: 0.42em 0.6em 0.42em 0;
}

tabledrag.module.css

/**
 * TableHeader behavior.
 *
 * @see tableheader.js
 */
table.sticky-header {
  background-color: #fff;
  margin-top: 0;
  z-index: 500;
  top: 0;
}

sticky-header.css

/**
 * Progress behavior.
 *
 * @see progress.js
 */
.progress {
  position: relative;
}
.progress__track {
  background-color: #fff;
  border: 1px solid;
  margin-top: 5px;
  max-width: 100%;
  min-width: 100px;
  height: 16px;
}
.progress__bar {
  background-color: #000;
  height: 1.5em;
  min-width: 3%;
  max-width: 100%;
}
.progress__description,
.progress__percentage {
  color: #555;
  overflow: hidden;
  font-size: .875em;
  margin-top: 0.2em;
}
.progress__description {
  float: left; /* LTR */
}
[dir="rtl"] .progress__description {
  float: right;
}
.progress__percentage {
  float: right; /* LTR */
}
[dir="rtl"] .progress__percentage {
  float: left;
}
.progress--small .progress__track {
  height: 7px;
}
.progress--small .progress__bar {
  height: 7px;
  background-size: 20px 20px;
}

progress.module.css

/* Throbber */
.ajax-progress {
  display: inline-block;
  padding: 1px 5px 2px 5px;
}
[dir="rtl"] .ajax-progress {
  float: right;
}
.ajax-progress-throbber .throbber {
  background: transparent url(../../../misc/throbber-active.gif) no-repeat 0px center;
  display: inline;
  padding: 1px 5px 2px;
}
.ajax-progress-throbber .message {
  display: inline;
  padding: 1px 5px 2px;
}
tr .ajax-progress-throbber .throbber {
  margin: 0 2px;
}
.ajax-progress-bar {
  width: 16em;
}

/* Full screen  throbber */
.ajax-progress-fullscreen {
  /* Can't do center:50% middle: 50%, so approximate it for a typical window size. */
  left: 49%;
  position: fixed;
  top: 48.5%;
  z-index: 1000;
  background-color: #232323;
  background-image: url("../../../misc/loading-small.gif");
  background-position: center center;
  background-repeat: no-repeat;
  border-radius: 7px;
  height: 24px;
  opacity: 0.9;
  padding: 4px;
  width: 24px;
}

ajax-progress.module.css

/**
 * Inline items.
 */
.container-inline div,
.container-inline label {
  display: inline;
}
/* Details contents always need to be rendered as block. */
.container-inline .details-wrapper {
  display: block;
}

container-inline.module.css

/* Display form elements horizontally. */
.form--inline .form-item {
  float: left; /* LTR */
  margin-right: 0.5em; /* LTR */
}
[dir="rtl"] .form--inline .form-item {
  float: right;
  margin-right: 0;
  margin-left: 0.5em;
}
.form--inline .form-item-separator {
  margin-top: 2.3em;
  margin-right: 1em; /* LTR */
  margin-left: 0.5em; /* LTR */
}
[dir="rtl"] .form--inline .form-item-separator {
  margin-right: 0.5em;
  margin-left: 1em;
}
.form--inline .form-actions {
  clear: left; /* LTR */
}
[dir="rtl"] .form--inline .form-actions {
  clear: right;
}

form--inline.module.css

/**
 * Prevent text wrapping.
 */
.nowrap {
  white-space: nowrap;
}

nowrap.module.css

/**
 * For anything you want to hide on page load when JS is enabled, so
 * that you can use the JS to control visibility and avoid flicker.
 */
.js .js-hide {
  display: none;
}

/**
 * For anything you want to show on page load only when JS is enabled.
 */
.js-show {
  display: none;
}
.js .js-show {
  display: block;
}

js.module.css

/**
 * Hide elements from all users.
 *
 * Used for elements which should not be immediately displayed to any user. An
 * example would be collapsible details that will be expanded with a click
 * from a user. The effect of this class can be toggled with the jQuery show()
 * and hide() functions.
 */
.hidden {
  display: none;
}

/**
 * Hide elements visually, but keep them available for screen readers.
 *
 * Used for information required for screen reader users to understand and use
 * the site where visual display is undesirable. Information provided in this
 * manner should be kept concise, to avoid unnecessary burden on the user.
 * "!important" is used to prevent unintentional overrides.
 */
.visually-hidden {
  position: absolute !important;
  clip: rect(1px, 1px, 1px, 1px);
  overflow: hidden;
  height: 1px;
  width: 1px;
  word-wrap: normal;
}

/**
 * The .focusable class extends the .visually-hidden class to allow
 * the element to be focusable when navigated to via the keyboard.
 */
.visually-hidden.focusable:active,
.visually-hidden.focusable:focus {
  position: static !important;
  clip: auto;
  overflow: visible;
  height: auto;
  width: auto;
}

/**
 * Hide visually and from screen readers, but maintain layout.
 */
.invisible {
  visibility: hidden;
}

hidden.module.css

/**
 * Float clearing.
 *
 * Based on the micro clearfix hack by Nicolas Gallagher, with the :before
 * pseudo selector removed to allow normal top margin collapse.
 *
 * @see http://nicolasgallagher.com/micro-clearfix-hack
 */
.clearfix:after {
  content: "";
  display: table;
  clear: both;
}

clearfix.module.css

/**
 * Text alignment classes.
 */
.text-align-left {
  text-align: left;
}
.text-align-right {
  text-align: right;
}
.text-align-center {
  text-align: center;
}
.text-align-justify {
  text-align: justify;
}

/**
 * Alignment classes (images, videos, blockquotes …).
 */
.align-left {
  float: left;
}
.align-right {
  float: right;
}
.align-center {
  display: block;
  margin-left: auto;
  margin-right: auto;
}

align.module.css

/*
 * Remove browser styles, especially for <buttons> and so on.
 */
.reset-appearance {
  -webkit-appearance: none;
  -moz-appearance: none;
  appearance: none;
  border: 0 none;
  background: transparent;
  padding: 0;
  margin: 0;
  line-height: inherit;
}

reset-appearance.module.css

/*
 * Contain positioned elements.
 */
.position-container {
  position: relative;
}

position-container.module.css

Lucasljj’s picture

+/* IE8 does not support :not() and :last-child. */

can we remove this advice? '-'

LewisNyman’s picture

We should, but in the issues where we've been moving and splitting up files, we've been avoiding making changes. Just to keep it simple. We can create issues to clean up all of these files individually.

pjbaert’s picture

Assigned: Unassigned » pjbaert
pjbaert’s picture

Assigned: pjbaert » Unassigned
Status: Needs work » Needs review
FileSize
22 KB
66.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,287 pass(es). View

I processed the change suggested in #50:

+++ b/core/modules/system/css/components/links.theme.css
@@ -0,0 +1,29 @@
+}
+button.link {
+  background: transparent;
+  border: 0;
+  cursor: pointer;
+  margin: 0;
+  padding: 0;
+  font-size: 1em;
+}

is now in link.theme.css

All suggestions of #53 are now in individual files.

Manjit.Singh’s picture

Issue tags: +Novice

Good work pjbaert !!

I think we can also move .tree-child's css to tree-child.css(new) file

div.tree-child {
  background: url(../../../misc/tree.png) no-repeat 11px center; /* LTR */
}
div.tree-child-last {
  background: url(../../../misc/tree-bottom.png) no-repeat 11px center; /* LTR */
}
[dir="rtl"] div.tree-child,
[dir="rtl"] div.tree-child-last {
  background-position: -65px center;
}
div.tree-child-horizontal {
  background: url(../../../misc/tree.png) no-repeat -11px center;
}

Also add

.tabledrag-toggle-weight-wrapper {
  text-align: right; /* LTR */
}
[dir="rtl"] .tabledrag-toggle-weight-wrapper {
  text-align: left;
}

in tabledrag.module.css file

tagging this a novice task..

manmohandream’s picture

FileSize
55.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch split_system_module_css-2395853-59.patch. Unable to apply patch. See the log in the details link for more information. View

Created new file tree-child.css and done changes mentioned in #58

Status: Needs review » Needs work

The last submitted patch, 59: split_system_module_css-2395853-59.patch, failed testing.

Manjit.Singh’s picture

+++ b/core/modules/system/system.libraries.yml
index 9f4a2aa..b59fd5b 100644
Binary files a/core/vendor/symfony/console/Resources/bin/hiddeninput.exe and b/core/vendor/symfony/console/Resources/bin/hiddeninput.exe differ

No need to add binary file here in this issue.

Also please include tree-child.css to info.yml file.

LewisNyman’s picture

It looks like the patch is #59 is missing a few of the changes from #57 as it's 10kb less. I would recommend working from the patch in #57.

Manjit.Singh’s picture

FileSize
66.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,654 pass(es). View

@lewis can you please verify now.

Manjit.Singh’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: -Novice

Thanks

  1. +++ b/core/modules/system/system.libraries.yml
    @@ -3,9 +3,54 @@ base:
    +      css/ajax-progress.module.css: { every_page: true, weight: -10, }
    +      css/align.module.css: { every_page: true, weight: -10, }
    ...
    +      css/container-inline.module.css: { every_page: true, weight: -10, }
    +      css/clearfix.module.css: { every_page: true, weight: -10, }
    +      css/details.module.css: { every_page: true, weight: -10, }
    +      css/form--inline.module.css: { every_page: true, weight: -10, }
    +      css/hidden.module.css: { every_page: true, weight: -10, }
    +      css/js.module.css: { every_page: true, weight: -10, }
    +      css/nowrap.module.css: { every_page: true, weight: -10, }
    +      css/position-container.module.css: { every_page: true, weight: -10, }
    +      css/progress.module.css: { every_page: true, weight: -10, }
    +      css/reset-appearance.module.css: { every_page: true, weight: -10, }
    +      css/resize.module.css: { every_page: true, weight: -10, }
    

    Some files are in the css folder and the rest are in the css/components folder. Can we move them all into the components folder?

  2. +++ b/core/modules/system/system.libraries.yml
    @@ -3,9 +3,54 @@ base:
    +      css/components/animated-throbber.module.css: {}
    +      css/components/fieldgroup.module.css: {}
    ...
    +      css/sticky-header.css: {}
    ...
    +      css/components/action-links.theme.css: {}
    +      css/components/breadcrumbs.theme.css: {}
    +      css/components/button.theme.css: {}
    

    There are loads of files that need the 'every_page true, weight: -10' options that the other files have

  3. +++ b/core/modules/system/system.libraries.yml
    @@ -3,9 +3,54 @@ base:
           css/system.module.css: { every_page: true, weight: -10, }
    

    This file is deleted so we need to remove it from the yml file

  4. +++ b/core/modules/system/system.libraries.yml
    @@ -3,9 +3,54 @@ base:
    +      css/components/tree-child.css: {}
    

    This file name should be tree-child.theme.css

gavin.hughes’s picture

Assigned: Unassigned » gavin.hughes
gavin.hughes’s picture

FileSize
67.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,726 pass(es). View

From #65

  1. Moved the css files to the components folder
  2. Im not sure which files should have the 'every_page true, weight: -10' or if they should they all have as a default? Looking at some of the declarations in core like seven etc don't have it applied
  3. removed system.module.css from the yml file
  4. renamed tree-child.css to tree-child.theme.css and updated the yml file accordingly
martin107’s picture

Status: Needs work » Needs review

triggering testbot

gavin.hughes’s picture

Assigned: gavin.hughes » Unassigned
LewisNyman’s picture

Status: Needs review » Needs work

Thanks!

Im not sure which files should have the 'every_page true, weight: -10' or if they should they all have as a default? Looking at some of the declarations in core like seven etc don't have it applied

They all should have them, because the CSS files we are replacing had them before. We want to ensure that these files are loaded before any other module CSS files.

gavin.hughes’s picture

FileSize
68.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,567 pass(es). View

Hi! No thank you! I applied that to all the css files

pjbaert’s picture

Status: Needs work » Needs review

Tnx @gavin.hughes

Please try to add an interdiff each time you add an updated patch. (see: https://www.drupal.org/documentation/git/interdiff )
It makes it much easier to review.
This looks fine at first sight. I'll try to review this in detail later today.

P.S. It's nice to know who is behind the username ;)

LewisNyman’s picture

Status: Needs review » Needs work

I ran this through visual regression testing and I found that we need to update the background-image url() paths because all files are now one folder level deeper.

LewisNyman’s picture

+++ b/core/modules/system/css/components/ajax-progress.module.css
@@ -0,0 +1,41 @@
+.ajax-progress-throbber .throbber {
+  background: transparent url(../../../misc/throbber-active.gif) no-repeat 0px center;

Eg. This path becomes url(../../../../misc/throbber-active.gif)

LewisNyman’s picture

Status: Needs work » Needs review
Issue tags: -drupaldevdays, -Needs visual regression testing
FileSize
68.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,818 pass(es). View
9.82 KB
3.26 MB
3.19 MB

I made the changes and ran the visual regression testing again. I ran it using three separate lists of URLs just to be sure we had enough coverage.

The last one includes a lot of screenshots! It took about 30 seconds enough for the browser to print it as a PDF. I'll upload it separately.

LewisNyman’s picture

FileSize
24.44 MB

25.6MB!

LewisNyman’s picture

I tested the patch with csslint and found that we had more csslint errors than before. I'm investigating...

LewisNyman’s picture

FileSize
16.72 KB
63.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,834 pass(es). View

OK that took ages but I found a few CSS files that were duplicates and also a few lines that were in two files. Thanks CSSlint!

I also went through and cleaned up the @file comments. If someone could have a look over that would be great.

davidhernandez’s picture

Status: Needs review » Needs work

I'm reviewing all this now. I'll make multiple comments, because I may loose my place.

+++ /dev/null
@@ -1,448 +0,0 @@
-.tabledrag-toggle-weight-wrapper {
-  text-align: right; /* LTR */
-}
-[dir="rtl"] .tabledrag-toggle-weight-wrapper {
-  text-align: left;
-}

These seemed to have disappeared. #58 requested they be moved, but they were lost? It is at the end of the tabledrag section of system.module.css.

davidhernandez’s picture

+++ /dev/null
@@ -1,603 +0,0 @@
-/**
- * Markup generated by tablesort-indicator.html.twig.
- */
-th.is-active img {
-  display: inline;
-}
-td.is-active {
-  background-color: #ddd;
-}

These are missing. I see in #47 that Lewis commented they can be removed because they are already in tablesort.theme.css; however, they are in there as .active, not .is-active.

davidhernandez’s picture

+++ /dev/null
@@ -1,603 +0,0 @@
-.pager__item {
-  display: inline;
-  padding: 0.5em;
-}
+++ b/core/modules/system/css/components/pager.theme.css
@@ -0,0 +1,18 @@
+.pager__item {
+  background-image: none;
+  display: inline;
+  list-style-type: none;
+  padding: 0.5em;
+}

Declarations for .pager__item changed, but I don't see any comments in this issue mentioning it.

davidhernandez’s picture

+++ /dev/null
@@ -1,603 +0,0 @@
-.menu-item {
-  padding-top: 0.2em;
-  margin: 0;
-}
-ul.menu a.is-active {
-  color: #000;
-}
+++ b/core/modules/system/css/components/menu.theme.css
@@ -0,0 +1,34 @@
+ul.menu li {
+  padding-top: 0.2em;
+  margin: 0;
+}
+ul.menu a.active {
+  color: #000;
+}

This was changed. No mention of it in the issue.

davidhernandez’s picture

+++ /dev/null
@@ -1,603 +0,0 @@
-.tabs a.is-active {
-  background-color: #eee;
-}

The .is-active class name was changed.

+++ b/core/modules/system/css/components/tabs.theme.css
@@ -0,0 +1,33 @@
+.tabs a.active {
+  background-color: #eee;
+}

Changed to .active.

davidhernandez’s picture

Some general issues with comments.

1) The language of the top of file comment blocks are inconsistent, “Visual styles for” and then some are just “Styles for”. Some use singular words, “throbber”, then plural, “throbbers”.
2) There is inconsistent spacing after the top of file comment blocks. Some have a blank space after the comment, some have two, some have nonet. There should be no blank space.
3) Can we get a follow up to fix inline comments? There are mistakes all over the place. Top of file comments should be fixed here though. They are being created by this issue.

Specific issues below:

+++ b/core/modules/system/css/components/animated-throbber.module.css
@@ -0,0 +1,22 @@
+ * @see autocomple.js

Typo.

+++ b/core/modules/system/css/components/collapse-processed.theme.css
@@ -0,0 +1,32 @@
+ * Visual styles for collapse processed.

Is this directly referencing the class name? If so, it should be the same, "collapsed-processed".

+++ b/core/modules/system/css/components/container-inline.theme.css
@@ -0,0 +1,18 @@
+/**
+ * Inline items.
+ */

Missing @file.

+++ b/core/modules/system/css/components/details.module.css
@@ -0,0 +1,9 @@
+/**
+ * Collapsible details.
+ *
+ * @see collapse.js
+ */

Missing @file.

+++ b/core/modules/system/css/components/details.theme.css
@@ -0,0 +1,22 @@
+/**
+ * Collapsible details.
+ *
+ * @see collapse.js
+ * @thanks http://nicolasgallagher.com/css-background-image-hacks/
+ */

Missing @file. @thanks is not a real thing is it? Don't add tokens if they aren't standard. Also, the thanks line is in this details file but not the others. Should it be?

+++ b/core/modules/system/css/components/more-link.theme.css
@@ -0,0 +1,13 @@
+ * Markup generated by #type 'more_link'.

More link has this technical detail. Can more help benefit from something similar, instead of the simple comment that is there?

+++ b/core/modules/system/css/components/node.theme.css
@@ -0,0 +1,8 @@
+/**
+ * @file
+ * Visual styles for node--unpublished.
+ */

If anything node related will go here in the future, this comment should more generic. Something like "Visual styles for nodes."

+++ b/core/modules/system/css/components/reset-appearance.module.css
@@ -0,0 +1,14 @@
+/*
+ * @file
+ * Utility class to remove browser styles, especially for <buttons> and so on.
+ */

The tag is not plural, and don't use "and so on." Try something simpler, like "Utility class to remove browser styles, especially for buttons."

+++ b/core/modules/system/css/components/table-select.theme.css
@@ -0,0 +1,14 @@
+ * TableSelect behavior.

I don't think this casing "TableSelect" is correct. The JS file itself says "Table select functionality."

+++ b/core/modules/system/css/components/tabledrag.module.css
@@ -0,0 +1,78 @@
+ * TableDrag behavior.

Something here with "TableDrag".

+++ b/core/modules/system/css/components/tablesort.theme.css
@@ -0,0 +1,11 @@
+ * Markup generated by tablesort-indicator.html.twig.

I'm not sure if there is a benefit to specifying the template where this comes from. That isn't in any other file comment.

+++ b/core/modules/system/css/components/textarea.theme.css
@@ -0,0 +1,11 @@
+ * Visual styles for resizeable textarea.

Add "a". "Visual styles for a resizable textarea."

+++ b/core/modules/system/css/components/tree-child.theme.css
@@ -0,0 +1,18 @@
+ * Visual styles for tree child.

This sentence needs work.

davidhernandez’s picture

Regarding files:

+++ b/core/modules/system/system.libraries.yml
@@ -3,9 +3,50 @@ base:
+      css/components/sticky-header.css: { every_page: true, weight: -10, }

The sticky file is the only one that does not have .module or .theme in its name.

+++ b/core/modules/system/system.libraries.yml
@@ -3,9 +3,50 @@ base:
+      css/components/table-select.theme.css: { every_page: true, weight: -10, }

Why does this file name have a hyphen when tabledrag and tablesort do not? They probably all should?

+++ b/core/modules/system/system.libraries.yml
@@ -3,9 +3,50 @@ base:
+      css/components/ajax-progress.module.css: { every_page: true, weight: -10, }
+      css/components/align.module.css: { every_page: true, weight: -10, }
+      css/components/animated-throbber.module.css: { every_page: true, weight: -10, }
+      css/components/fieldgroup.module.css: { every_page: true, weight: -10, }
+      css/components/container-inline.module.css: { every_page: true, weight: -10, }

I don't think the trailing comma is necessary. We do that for an array in PHP, but I don't think that is done in YAML files. I looked through some other files in core and don't see it being done.

davidhernandez’s picture

Last comment for now, I think. General things.

1) Why do we have two things that animate throbber? .ajax-progress and animater-throbber.module.css
2) Do we really need separate files for all the little things? nowrap is in its own file. clear fix is its own file. There are a lot of files that have one thing in them. It would be worth separating if they will later be conditionally included, or there was a reason to remove them. Is it necessary for something like nowrap or all the others?
3) The breadcrumb file name is plural but the class name is singular.

LewisNyman’s picture

Assigned: Unassigned » LewisNyman
LewisNyman’s picture

Assigned: LewisNyman » Unassigned
Status: Needs work » Needs review
FileSize
15.61 KB
62.98 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,267 pass(es). View

@davidhernandez++++

Thanks for the thorough review. I've been distracted by usability stuff. Rerolling this was tricky, but I think I caught the recent changes to system.theme.css and system.libraries.yml: #2472431: Do not load normalize.css in all themes, load it in Classy and #2501903: inline form errors classnames to follow namestandard

Most of your comments were simple fixes, here are a few comments for the less obvious ones.

More link has this technical detail. Can more help benefit from something similar, instead of the simple comment that is there?

more-help-link has been removed, so I deleted the file. See https://www.drupal.org/node/2462579

Do we really need separate files for all the little things? nowrap is in its own file. clear fix is its own file. There are a lot of files that have one thing in them. It would be worth separating if they will later be conditionally included, or there was a reason to remove them. Is it necessary for something like nowrap or all the others?

Yeah, the long term aim here is consistency and findability. It should always be obvious where to find the CSS for a class. That means each class (or CSS component) should get it's own file.

Why do we have two things that animate throbber? .ajax-progress and animater-throbber.module.css

Good point, I've renamed the animater-throbber.module.css to match the class closer. autocomplete-loading.module.css

The breadcrumb file name is plural but the class name is singular.

Good catch, renamed to match the class.

davidhernandez’s picture

Assigned: Unassigned » davidhernandez
Status: Needs review » Needs work

A few things, but nothing serious. Some of the problems with comments I'm ok with saying don't have to be fixed if they were copied from the original file, but comment blocks at the top of files should be fixed. They are all new files and we are creating those problems here.

1) There is inconsistent spacing after the top comment blocks. There should be a single space between the bottom of the comment and the first line of code. A few have no space, and one or two have two spaces.

Like this:

+++ b/core/modules/system/css/components/action-links.theme.css
@@ -0,0 +1,43 @@
+/**
+ * @file
+ * Styles for link buttons and action links.
+ */
+
+.action-links {

Not this:

+++ b/core/modules/system/css/components/align.module.css
@@ -0,0 +1,31 @@
+/**
+ * @file
+ * Alignment classes for text and block level elements.
+ */
+.text-align-left {

2) The "specificity" is used a few times in comments throughout the files and it is mis-spelled as "specifity". I know this is copied over, but I'd like to fix this.

For example, in action-links.theme.css

/* This is required to win over specifity of [dir="rtl"] ul */

3) ajax-progress.module.css there is an accidental double-space in a comment.

/* Full screen throbber */

4) There are a couple places where ellipsis is used in a comment. We don't do that anymore in the UI or comments. See https://www.drupal.org/node/2279105 where most of it was removed. Replace the ellipsis (...) with an ", etc."

Instead of this:

/**
 * Alignment classes for block level elements (images, videos, blockquotes …).
 */

This:

/**
 * Alignment classes for block level elements (images, videos, blockquotes, etc.)
 */

5) clearfix.module.css is missing the @file.

6) more-link.theme.css has two blank lines at the end of file instead of one.

7) stikcy-header.module.css the top comment has “TableHeader”. It should be "Table header". The other files, like table drag were fixed.

8) textarea.theme.css resizable is spelled wrong.

9) text area wrapper was added to both textarea.theme.css and resize.module.css. It should only be in textarea.theme.css, so remove it from resize.module.css. (I confirmed this with Lewis on IRC.)

+.form-textarea-wrapper textarea {
+  display: block;
+  margin: 0;
+  width: 100%;
+  box-sizing: border-box;
+}

10) tree-child.theme.css All of its CSS came from system.module.css. I assume it should be tree-child.module.css. I see it was commented on in #65 but involved changing it from tree-child.css, so I'm assuming this was just a mistake.

There are some other issues with spacing here or there that I could comment on, but most of those mistakes are carried over from the original files and may make fixing this and reviewing it muddier, so I'm ok leaving it.

These additional corrections are simple. I'm getting a new contributor at the GovCon sprint to work on it, so DON'T TOUCH. I'm assigning to myself to put a hold on it. If I don't fix it by tomorrow I'll comment and unassign myself.

onelittlebecca’s picture

Hey, I am at govcon and am going to work on this.

onelittlebecca’s picture

Status: Needs work » Needs review
FileSize
62.88 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,835 pass(es). View
5.17 KB

#89 -- all the concerns have been addressed in my patch

kgoel’s picture

helped onelittlebecca with her first patch.

davidhernandez’s picture

Assigned: onelittlebecca » Unassigned
Status: Needs review » Needs work

From my list in #89.

1) Missed a few files. link.theme.css has a double space, progress.module.css is missing a space, tabledrag.module.css is missing a space.
2) "specifity" missed in breadcrumb.theme.css
3) Done.
4) Done.
5) Done.
6) Done.
7) Done.
8) Done.
9) Done.
10) Done.

davidhernandez’s picture

Assigned: Unassigned » davidhernandez

Finding someone to work on this.

alisonjo2786’s picture

I'm working on this issue at Drupal GovCon.

alisonjo2786’s picture

Status: Needs work » Needs review
FileSize
62.88 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2395853-96.patch. Unable to apply patch. See the log in the details link for more information. View
1.59 KB

Fixed #1 and #2 from #93, with help from @kgoel. (#drupalgovcon)

davidhernandez’s picture

Assigned: alisonjo2786 » Unassigned
Issue summary: View changes
davidhernandez’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

All of my points have been addressed.

Adding a suggested commit message with me and Kalpana. Kalpana mentored onelittlebecca and alisonjo2786 during the GovCon sprint.

davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

I'm assuming this will need a reroll after #2426967: Feed icon should be a CSS background image not a image file. Will feed icon need its own file or add it to icons.theme.css?

davidhernandez queued 96: 2395853-96.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 96: 2395853-96.patch, failed testing.

davidhernandez’s picture

Ayup. This was added to system.theme.css

+++ b/core/modules/system/css/system.theme.css
+ * Feed Icon.
+ * Markup generated by feed-icon.html.twig.
+ */
+.feed-icon {
+  background: url(../../../misc/feed.png) no-repeat;
+  overflow: hidden;
+  text-indent: -9999px;
+  display: block;
+  width: 16px;
+  height: 16px;
+}
+
+/**

Moving it to icons.theme.css seems reasonable. It has "icon" in the class name. "action-links" and "button-actions" are in the same file, for example.

LewisNyman’s picture

Yep sounds good to me

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
63.29 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,960 pass(es). View
davidhernandez’s picture

+++ b/core/modules/system/css/components/icons.theme.css
@@ -0,0 +1,22 @@
+

Is that an errant space? If not, and it is worth separating them, it might be worth a comment.

davidhernandez’s picture

+++ b/core/modules/system/css/components/icons.theme.css
@@ -0,0 +1,22 @@
+}
+
+.feed-icon {

That last code comment probably wasn't clear.

LewisNyman’s picture

FileSize
435 bytes
63.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,925 pass(es). View

Ah good point.

Wim Leers’s picture

Related question: is there a plan (in a follow-up issue) to also split this into many separate asset libraries as well? That'd open the door to inlining the critical asset libraries only, which is crucial for excellent front-end performance on mobile devices.

This is just a related question. Please don't start doing that on this issue. If it's too distracting to answer here, feel free to say so. It's just that this issue, in splitting our big CSS into many smaller CSS files, actually makes it easier to do inlining of critical asset libraries.

LewisNyman’s picture

@Wim Leers There isn't a follow up issue, but we did discuss at Drupalcon that this issue would enable more fine grained loading. We still have a lot of #2489460: [Meta] Move module.theme.css files to Classy or Seven to do. I don't know which release would be a candidate target for that.

davidhernandez’s picture

Yeah, the aim is to split the files up and then eventually work out the conditional inclusion of them, but we may not get to the second part before release. Hopefully, we can do them in a point release.

At the very least, the split up should be beneficial in letting themes pick and choose which files to keep. The libraries-override would also be great for this (#2451411: Add libraries-override to themes' *.info.yml) but I don't know if that will get in before release either.

Wim Leers’s picture

Okay, thanks! :)

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

I compared #96 to #108 and all appears in order. The feed icon styles were moved.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/search/src/Tests/SearchQueryAlterTest.php
    @@ -47,7 +47,7 @@ function testQueryAlter() {
    -    $this->assertText('article', 'Article is in search results');
    -    $this->assertNoText('page', 'Page is not in search results');
    +    $this->assertText('test article', 'Article is in search results');
    +    $this->assertNoText('test page', 'Page is not in search results');
    

    This is not a great change. At the moment AssertContentTrait::getTextContent() is including the css @import statements. This needs to be fixed. However, enough discussion has occurred on this issue. Created #2546582: AssertContentTrait::getTextContent() includes the @import css statement, that is nuts.

  2. +++ b/core/modules/system/css/components/icons.theme.css
    @@ -0,0 +1,21 @@
    +  background: url(../../../misc/feed.png) no-repeat;
    

    Needs another level of ../

alexpott’s picture

I've been thinking about this some more - I think considering the fact that this is going to add @import's everywhere in testing we need to get #2546582: AssertContentTrait::getTextContent() includes the @import css statement, that is nuts in first.

alexpott’s picture

+++ b/core/modules/search/src/Tests/SearchQueryAlterTest.php
@@ -47,7 +47,7 @@ function testQueryAlter() {
-    $this->assertText('article', 'Article is in search results');
-    $this->assertNoText('page', 'Page is not in search results');
+    $this->assertText('test article', 'Article is in search results');
+    $this->assertNoText('test page', 'Page is not in search results');

This change is unnecessary now.

davidhernandez’s picture

Great. Can someone upload a new patch with an interdiff removing the code per #114.1 and fixing #114.2? I'll review it.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
62.51 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,322 pass(es). View
1.26 KB

Addressing feedback in #114.

Over to you @davidhernandez, pending a successful test run :)

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Verified that the feed icon on the home page appears with the correct path.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cf6104b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to issue summary.

  • alexpott committed cf6104b on 8.0.x
    Issue #2395853 by LewisNyman, Manjit.Singh, nlisgo, pjbaert, Branislav...
LewisNyman’s picture

WOOHOO! Amazing. Well done everyone. Now for the easy work ;-)

Wim Leers’s picture

alexpott’s picture

This needs a followup we still have references to system.module.css and system.theme.css in core.

  • alexpott committed 55f6f42 on 8.0.x
    Issue #2552187 by Wim Leers, LewisNyman: Follow-up for #2395853:...
davidhernandez’s picture

Status: Fixed » Closed (fixed)

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

LewisNyman’s picture

FYI this commit accidentally reverted #2486431: Progress bar starts at 100%