Clean up the CSS for the Node module

Decided to give this a go, as far as I could tell, the css for node simply needed moving into the node.theme.css file, and corrections of the paths in node.info for the stylesheets were made. I'm keen to get involved and this is my first low-hanging-fruit attempt - so pointers are well appreciated!

I haven't done anything to the -rtl.css file, as I wanted to check - does that need to be abstracted into module.theme-rtl.css and module.admin-rtl.css or is there a new way of doing rtl css as part of the cleanup?

Followed the example put at module.system with the -rtl suffixes, and added the node-rtl.css refactoring to the patch.

Just fixed the failing test. Since you renamed node.css, to node.theme.css, the test that referred to node.css failed...

That patch strangely fails too - any ideas?

Catch pointed out that I had forgotten to add the new css files in the above patch. Re-rolled...

Hmm ... just noted the discussion in the Comment cleanup (http://drupal.org/node/1216976#comment-4846766) pondering having simply .published and .unpublished. Does anyone, Jacine included, have any input on this?

Hey jvc26 & brianV! Thanks for getting started on this. Great work so far. ;)

+++ b/modules/node/node.admin-rtl.cssundefined
@@ -0,0 +1,14 @@
+
+#node-admin-content dl.multiselect dd .form-item label {
+  display: block;
+  float: right;
+  width: 6em;
+  font-weight: normal;
+}

This can be removed. Somehow either we forgot about this, or the patch wasn't committed correctly when we removed that horrid dl.multiselect code from Drupal 7.

+++ /dev/nullundefined
--- /dev/null
+++ b/modules/node/node.admin-rtl.cssundefined

Hmm, we can't possibly have a node.admin-rtl.css without a node.admin.css and with this patch, the RTL file wouldn't actually load anywhere. I think that we messed this up and forgot to remove this entire file in D7 somehow in the same patch I mentioned above, but we need to double-check if #node-admin-buttons exists anywhere, just in case.

+++ b/modules/node/node.theme.cssundefined
@@ -0,0 +1,10 @@
+.node-unpublished {
+  background-color: #fff4f4;

Yeah, so I would definitely like to change this to just .unpublished and stick it in system.theme.css, but I didn't get any feedback in the other issue. :(

What do you think?

+++ b/modules/node/node.theme.cssundefined
@@ -0,0 +1,10 @@
+.preview .node {
+  background-color: #ffffea;

It would also be cool if we could stop adding a wrapper div here and just add a class indicating that the node is being previewed, but since that's a pretty significant change we can do that in a follow-up.

15 days to next Drupal core point release.

Ok, so that whole node.admin-rtl.css file should be removed. I double checked, and it looks like there was a mixup when code was being removed/moved to other files. That code block was supposed to be removed in this patch: #732914: Improve the markup/CSS for content and user filter forms.

Looking at the css in the current 8.x version, it looks like another patch already removed some of the css compared to when the first patch on this issue was created.
Attached is a new patch reflecting these changes, and incorporating the feedback from Jacine.

Overview of the changes in this patch:
- .node-unpublished has been changed to .published and was moved to system.theme.css. I will mention this in issue #1216976 so that these changes can also be incorporated there if this change gets through.
- updated Bartik to reflect the update from .node-unpublished to .unpublished.
- The selector .preview .node was changed to .node-preview since that seems the more appropriate class to use. I did not remove the preview div (yet) as it might still have a use if you want to target the entire preview.
- Moved all remaining css to admin.css and made sure this file is only loaded in the right places in the backend.

Changing status to 'needs review'.

Woopsie, needed to update one of the tests apparently.

I assume the test from the bot will need to be updated so putting this to 'needs review'.

New patch attached also moves .node-preview to .preview in system.theme.css, to be in line with patch in #1216976: Clean up the CSS for Comment module.

The 'preview' div has been removed to avoid conflict with the .preview class now added to nodes and comments.

Thanks for all your work on this issue. Looks like we may need to patch a test here. The test that is failing is:
CascadingStylesheetsTestCase->testAddCssFileWithQueryString()
If we're changing something that will change what that test should expect, we'll need to include a fix for the test in the patch.

Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades).

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Rerolled the patch for the new folder structure. Also added a @file header for the node.admin.css file.

An update for the simpletest module is included in the patch, but I assume I can't add a patch for the testbot myself?

@jyve: Testbot runs drupal with your patch applied, so if your patch includes a change to a test, then testbot's tests run with that change. :)

aha, I indeed missed a test. New patch should be ok!

Anyone care to review the patch in #26? It would be nice to get this in together with the comment patch in #1216976: Clean up the CSS for Comment module

+++ b/core/modules/node/node.admin.cssundefined
@@ -0,0 +1,12 @@
+td.revision-current {
+  background: #ffc;
+}

can it without "td" ?

+++ b/core/themes/bartik/css/style.cssundefined
@@ -658,11 +658,11 @@ ul.links {
+.unpublished {
   margin: -20px -15px 0;
   padding: 20px 15px 0;
 }

removed this one and no different

+++ b/core/themes/bartik/css/style.cssundefined
@@ -658,11 +658,11 @@ ul.links {
+.unpublished .comment-text .comment-arrow {
   border-left: 1px solid #fff4f4;
   border-right: 1px solid #fff4f4;
 }

why the color here is diff to other status. I can see two 1px straight line there..

and the color value:

Note that colors in RGB notation (e.g., #FFF) are preferred in uppercase, whereas non-color values should be in lowercase.

http://drupal.org/node/302199
15 days to next Drupal core point release.

Thanks for the good feedback droplet!

- The color codes in node.admin.css have been updated to uppercase.
- The td could indeed be removed in node.admin.css, tested this against Bartik.
- There was an issue with moving the unpublished to system.theme.css since the pink background color was being overwritten in Bartik. This explains why the borders on the comment-arrow did not make since. I fixed this and it does make sense now.
- the margin/padding in Bartik does have an affect, it makes sure the content does not stick against the border of the 'unpublished' background color. This was propably invisible too due to the error mentioned in the previous line.
- I've opened a new issue to get Bartik to follow Drupal CSS coding standards, so that we can fix the lowercase color codes there.

Oh, this dropped off my radar without the tags. Anyway, tagging for this sprint. Thanks for the work on this so far @jyve.

Since there has been a good agreement to change color codes to lowercase (#1360790: CSS coding standards: Recommend lowercase not UPPERCASE hex colors), I have updated the patch accordingly.

#34: node_css_cleanup-1217012-34.patch queued for re-testing.

I think node.admin.css is missing :)

ow man, thx for pointing it out aspilicious :)

Added the missing file

Tested and looks good to me, only note that the css here:

+  // Add the required css.
+  drupal_add_css(drupal_get_path('module', 'node') . '/node.admin.css');

is already attached here:

+    '#attached' => array (
+      'css' => array(drupal_get_path('module', 'node') . '/node.admin.css'),
+    ),

+++ b/core/modules/node/node.pages.incundefined
@@ -432,7 +432,10 @@
+  // Add the required css.
+  drupal_add_css(drupal_get_path('module', 'node') . '/node.admin.css');
+

In admin.css there is 1 line and that one covers revisions. And in node_revision_overview we alrdy attach admin.css . SO I think this must be a left over.

15 days to next Drupal core point release.

Comming from #862854: No styling for sticky

we should also do:

 /**
- * Override or insert variables into the node template.
- */
-function bartik_preprocess_node(&$variables) {
-  if ($variables['view_mode'] == 'full' && node_is_page($variables['node'])) {
-    $variables['classes_array'][] = 'node-full';
-  }
-}

I'm on it.

Final one? :)

#44 look solid! I'm sleepy so I'm gonna check again in the morning when I'm a bit more sharp

hi aspilicious, looks like your patch will need to be rerolled, it no longer applies to the current Drupal version. I will be happy to review it once you re-roll.

reroll

review... Please...

Patch tested, and looks perfect to me.

Let's get this one in before it needs to be rerolled!

go go!

It looks like this patch introduces inconsistencies. Sometimes we prefix things with "node-" but other times we don't?

Dries we decided to move some styling to the system module

--- a/core/modules/system/system.theme.css
+++ b/core/modules/system/system.theme.cssundefined
@@ -38,6 +38,16 @@
 }
 
 /**
+ * Publishing status.
+ */
+.unpublished {
+  background-color: #fff4f4;
+}
+.preview {
+  background-color: #ffffea;
+}

That way we can reuse the classes for user, comments, ...
So they aren't realy part of "node" aymore thats why we removed the prefix on those.

.revision is still part of node, though. That seems like it should be in system.module as well. (Or actually should all those be in entity.module?)

At the moment we don't have revision support yet in core for entities, only for nodes...
Unpublished and preview are not rly part of the entity structure entity

Dries does make a good point.

I'm not a fan of the typical node classes either. All of them can be generalized, and especially the 'node-teaser' one is too hard-coded since there can be more than two View Modes from a technical point of view.

The new patch attached leaves us with the following classes:

node
node-$type
promoted
sticky
unpublished
view-mode-$view_mode (e.g. view-mode-teaser of view-mode-full)
preview

As we move more to entities, I am sure some classes will be able to move to that module.
All feedback is appreciated.

#55: node_css_cleanup-1217012-56.patch queued for re-testing.

What does this patch need to advance? Manual / visual confirmation?

This patch needs testing, and agreement on the generalizing of the classes.

@jyve as in manual testing or do we need to write more functional tests? I'll help with either. I'll manually test this patch Saturday if no one else gets to it first.

#55: node_css_cleanup-1217012-56.patch queued for re-testing.

* patch applied cleanly on latest checkout of 8.x branch
* inspected the patch in a visual diff for errors/omissions of topics mentioned in this thread
* tested bartik changes and all classes are being applied properly on teasers

all good, patch has been manually reviewed.. there is a test in there for common.inc, which I honestly don't know what to do with, I'll leave it up to cosmicdreams to give it the final OK on Saturday.

Cool, thanks @KrisBulman

Giving this a manual test as well:

Test with: Bartik & Stark
Created nodes
Previewed nodes
Edited nodes
Deleted nodes

Everything works and looks fine. No regressions discovered.

So... The "agreement on generalizing the node classes" mentioned in #59, #55, and #51 -- should we get the Markup, Theme, and/or Node maintainers weigh in before considering this to be RTBC?

demoting while we wait for those folks to weigh in.

This patch looks good to me. It's what I've been doing on D7 sites. Thanks for sticking with this @jyve and for testing @cosmicdreams.

Putting this one on the jhodgdon pile, since it's a clean-up/coding standards task.

#55: node_css_cleanup-1217012-56.patch queued for re-testing.

This doesn't look right to me, from bartik/css/styles.css:

-.node-sticky {
+.view-mode-teaser.sticky {

Shouldn't there be a space before .sticky? Or maybe not... Isn't the sticky class going on the same div as the view-mode-teaser class? Maybe this some relatively new CSS thing I don't know about, where you can stick two classes together, but I've never heard of it. Please tell me I'm wrong. :)

Provisionally "needs work" until someone corrects my understanding (or the patch).

@jhodgdon That's on purpose. :) Both of those classes are on the same div. It's not new at all, but we've been unable to use it because of having to support IE6. All browsers, except IE6 understand this (for ID's too), and it's actually one of the main points of the patch and all the name changes. Here's some more information on it: http://css-tricks.com/multiple-class-id-selectors/ Let me know if you need more info. :)

More links for ya:

• http://www.w3.org/TR/CSS2/selector.html#class-html
• http://www.quirksmode.org/css/contents.html#t14
• http://www.quirksmode.org/css/multipleclasses.html

Cool! A learning moment for me. Thanks for the information. :)

Committed to 8.x.

Sure :D Thanks!

Removing the sprint tag :D