merge node.*.css files into node.admin.css

Library drupal.node.admin (node.admin.css) - contains theme css, used for overview table of older revisions of a node.
Libraries form, drupal.node (node.module.css) - contains layout css, used for node form.
Library drupal.node.preview (node.preview.css) - contains theme css, used for single node in preview.
It look logical to keep all three files.

It seems like the layout this code is duplicated in node.module.css? We could remove a lot of code from there as well.

The Proposed resolution in the IS is to merge all 3 of them. So I removed node.module.css instead.

... Ignore #4

Thanks! We need screenshots in Stark/Seven/Bartik to show everything still looks right, as we need a screenshot of Stark without any of the .theme.css styling to show that it is still functional.

Now we have four libraries that all call the same one file, maybe we can merge them together into one library?

Sorry for the following nitpicks >:P

+++ b/core/modules/node/css/node.admin.theme.css
@@ -73,3 +68,26 @@
+/* node preview conttainer is places*/

Maybe we should rephrase and fix typo in this comment. Also comments should start with capitals, end with a period and have an space between comment tags.

+++ b/core/modules/node/css/node.admin.theme.css
@@ -73,3 +68,26 @@
+/* revision color */

Comments should start with capitals and end with a period.

Fixed a missing period in a comment block and removed periods in inline comments. Not sure if they should have periods or not. I haven't found any info about it. Any ideas?

The rest of the patch seems to be ok and it works as expected. Check screenshots:

Preview BEFORE

Preview AFTER

Node BEFORE

Node AFTER

@rteijeiro Doxygen recommends full stops on the first line of a comment. I think the easiest rule to follow here is to put a full stop on every comment.

I've updated the CSS coding standards examples so they consistently use a full stop.

Thanks for all the screenshots. It looks like we've fixed all the concerns here. I manually tested it again then just to make sure we didn't loose any styling. It looks good.

The new css has lots of additional css. Surprising.

This looks great, just a couple of questions:

1. +++ b/core/modules/node/css/node.admin.theme.css
   @@ -73,3 +68,26 @@
   +@media only screen and (min-width: 36em) {
   +  .node-preview-container .form-type-select {
   ...
   +/* Revision color. */
   +.revision-current {
   +  background: #ffc;
   +}
   

   Should this go above the @media query? Not sure the pattern we are using here again...

2. +++ b/core/modules/node/node.libraries.yml
   @@ -2,7 +2,7 @@ drupal.node:
        layout:
   -      css/node.module.css: {}
   +      css/node.admin.theme.css: {}
   

   Shouldn't this be in a theme library category or something? Just guessing...

2. I don't know the rules around *.theme.css and theme: in yaml Maybe there is a rule you or @LewisNyman can point me to?

1. The issue summary indicates all 3 nodes.*.css files are merged into a single node.admin.css, but the patch leaves node.module.css. Is this on purpose?
2. node.admin.theme.css contains box-sizing, float, width attributes that deliver layout. This seems inconsistent with the SMACCS policy. Can you include a comment why these attributes are in a .theme file instead of a .module file?
3. @LewisNyman in #3
   

   It seems like the layout this code is duplicated in node.module.css? We could remove a lot of code from there as well.

   There is still a lot of duplicated css between node.module.css and node.admin.css.

I am at the drupal south sprint and going to update the issue summary

I applied the patch in #34 and noticed that node.module.css still exists, this should be removed.
The only file that remains then is node.admin.theme.css, which seems inconsistent with the issue summary, should this be called node.admin.css ?

Fixing which CSS files are created/named/deleted.

It looks like this introduces a problem with the node preview bar. Because the node preview page is is not an admin page, moving the CSS into the admin theme breaks the layout of the node preview bar. I think we should keep node preview in it's own file and just convert node.module.css to node.admin.css.

Before:

After:

In reading up on the docs to answer #43, I've realized that my change in #42 to answer #39 is actually incorrect -- the file should be named node.admin.theme.css (point 4 on #2016305: Architect CSS: Rename files to match CSS file naming convention).

Keeping node.preview.css breaks the naming conventions from what I could see. I think the correct answer would be to fix the preview page so that it loads the admin CSS.

I am worried that if we load all the admin CSS on a frontend page we might introduce loads of regressions. What if we put the preview CSS inside a node.theme.css file?

That sounds like a good compromise. I've basically renamed node.preview.css to node.theme.css, updated node.libraries.yml, and removed node.module.css that #34 missed.

Ok, I've taken screenshots of the node preview bar in Bartik and the node edit form in Seven and Classy to show there are no UI changes and added them to the issue summary. Thanks.

Creating a node is not an necessarily an administrator task - i'm not sure that node.admin.css is the correct name.

I don't think the designation is role based, it's mode/screen based. The node module's primary function is to create, manage, and display nodes. Following the guidelines outlined at https://www.drupal.org/node/2016305:

• the node.theme.css is to "make the functionality aesthetically pleasing" - in this case, the functionality is the node Preview bar, which will interact with the non-admin theme
• the node.admin.theme.css is to "make the module's admin screens aesthetically pleasing" - in this case, I don't think it's much of a stretch to call the node create and edit pages the "admin side" of node.module.

@akalata yep I can buy that.

So whilst I buy it - I don't completely.

The preview code is in node.theme.css after this patch. Because it appears in the main theme - but the same can be true of the node edit css.

If we put the edit CSS in admin, we should put preview in too. Preview can only be got to from edit.

Rerolled patch

From reading the IS, this is a task not a bug.

#58 patch failed for me.

Lenovo@LAPTOP-PDE747K8 MINGW64 /c/xampp/htdocs/drupal-2421365 (2421365-merge-node.css-files)
$ git apply -v merge_node_css_files-2421365-58.patch
Checking patch core/modules/node/css/node.admin.css...
Checking patch core/modules/node/css/node.admin.theme.css...
Checking patch core/modules/node/css/node.module.css...
error: while searching for:
/**
 * @file
 * Styles for administration pages.
 */

/**
 * Node add/edit form layout
 */

/* Narrow screens */
.layout-region {
  box-sizing:         border-box;
}

/* Wide screens */
@media
  screen and (min-width: 780px),
  (orientation: landscape) and (min-device-height: 780px) {

  .layout-region-node-main,
  .layout-region-node-footer {
    float: left; /* LTR */
    width: 65%;
    padding-right: 2em; /* LTR */
    box-sizing: border-box;
  }

  [dir="rtl"] .layout-region-node-main,
  [dir="rtl"] .layout-region-node-footer {
    float: right;
    padding-left: 2em;
    padding-right: 0;
  }

  .layout-region-node-secondary {
    float: right; /* LTR */
    width: 35%;
  }

  [dir="rtl"] .layout-region-node-secondary {
    float: left;
  }

  /* @todo File an issue to add a standard class to all text-like inputs */
  .layout-region-node-secondary .form-autocomplete,
  .layout-region-node-secondary .form-text,
  .layout-region-node-secondary .form-tel,
  .layout-region-node-secondary .form-email,
  .layout-region-node-secondary .form-url,
  .layout-region-node-secondary .form-search,
  .layout-region-node-secondary .form-number,
  .layout-region-node-secondary .form-color,
  .layout-region-node-secondary textarea {
    box-sizing:         border-box;
    width: 100%;
    max-width: 100%;
  }
}

/**
 * The vertical toolbar mode gets triggered for narrow screens, which throws off
 * the intent of media queries written for the viewport width. When the vertical
 * toolbar is on, we need to suppress layout for the original media width + the
 * toolbar width (240px). In this case, 240px + 780px.
 */
@media
  screen and (max-width: 1020px) {

  .toolbar-vertical.toolbar-tray-open .layout-region-node-main,
  .toolbar-vertical.toolbar-tray-open .layout-region-node-footer,
  .toolbar-vertical.toolbar-tray-open .layout-region-node-secondary {
    float: none;
    width: auto;
    padding-right: 0;
  }
}

error: patch failed: core/modules/node/css/node.module.css:1
error: core/modules/node/css/node.module.css: patch does not apply
Checking patch core/modules/node/css/node.preview.css...
error: while searching for:
/**
 * @file
 * Styles for node preview page.
 */

.node-preview-container {
  position: fixed;
  z-index: 499;
  width: 100%;
  padding: 10px;
  box-sizing: border-box;
}

@media only screen and (min-width: 36em) {
  .node-preview-container .form-type-select {
    margin-left: 25%; /* LTR */
  }
  [dir="rtl"] .node-preview-container .form-type-select {
    margin-right: 25%;
    margin-left: 0;
  }
}

error: patch failed: core/modules/node/css/node.preview.css:1
error: core/modules/node/css/node.preview.css: patch does not apply
Checking patch core/modules/node/css/node.theme.css...
Checking patch core/modules/node/node.libraries.yml...
Hunk #2 succeeded at 13 (offset -2 lines).
Checking patch core/modules/node/src/Controller/NodeController.php...
error: while searching for:
      '#rows' => $rows,
      '#header' => $header,
      '#attached' => array(
        'library' => array('node/drupal.node.admin'),
      ),
    );


error: patch failed: core/modules/node/src/Controller/NodeController.php:241
error: core/modules/node/src/Controller/NodeController.php: patch does not apply

Re rolled the above patch for Drupal 9.4.x

Patch failed. Don't see any answer for #35 in the issue summary.

Fixed Drupal CS issues of patch #78, still needs work for issue summary update.

Fixing Drupal CS issues of patch #80.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Hiding patches.

The MR merges the node preview CSS into node.css but leaves the library intact, I don't see a reason to merge the preview CSS - it's only used on preview, which is in a controller and attaches the preview library anyway.

It seems like node.admin.css, which only contains a background colour, could possibly be removed altogether rather than merged?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

tried several patches bot not applied create my own MR please check added all node folder css inside node.admin.css

I think I'm missing something, but why exactly do we want to combine all CSS into a single file? Currently these CSS files are split into separate libraries (although node.module.css is in 2 different libraries) which are added in different parts of the CMS depending on where they're needed. Combining all CSS into 1 file will mean adding CSS to places that it's not needed. Isn't it better to only add it where it's needed?

We also have many themes overriding or extending these libraries to add their own CSS, we would have to very carefully coordinate releases between all of these otherwise there will be visual regressions. Furthermore, it means contrib would have to have a new major/minor targeting the specific core release this is changed in.

The effort doesn't seem worth it IMO, what are we trying to gain here? Even though the CSS is all admin-ish related, it's for very different use cases.