Clean up the CSS for Book module

tag

taking this one

split up the css so its in admin & theme

cleaned out the stuff from #book-admin-edit have a hard time justifieng its existence or am i wrong there ?

Woot! Happy that you are working on this Morten! Go go go :)

Forgot the admin.theme.css file

forgot the book.theme.css file

newlines added ...

Okay, just reviewed this and found a couple of issues with the patch:

1. It's not properly deleting the book.css and book-rtl.css files for some reason. Did you delete the contents of the file and not the file itself? I tried this using the git add -N for the new files and deleted the old ones and it worked properly for me.
2. The RTL file should be in the patch. It should be changed to book.theme-rtl.css.
3. _book_admin_table_tree() doesn't seem like the right place to load book.admin.css. I'm going to ask around about this.

The rest looks good to me! Go Morten go! :D

+++ b/modules/book/book.admin.inc
@@ -174,6 +174,7 @@ function _book_admin_table($node, &$form) {
+  drupal_add_css(drupal_get_path('module', 'book') . '/book.admin.css');

This should use the #attached syntax.

this should do it

okay lets try again
have no clue why evil test bot dont give me any love

Yeah, hopefully that one works... We still need to deal with adding the book.admin.css file. I don't think that function is where it should be added, and @tstoeckler's right that it should probably use #attached.

yeah tried to get that one in but evil bot keept messing with me

+++ b/modules/book/book.admin.inc
@@ -174,6 +174,7 @@ function _book_admin_table($node, &$form) {
 function _book_admin_table_tree($tree, &$form) {
+  drupal_add_css(drupal_get_path('module', 'book') . '/book.admin.css');

This should go into _book_admin_table() instead, as the _tree helper calls itself recursively for each level of the entire book tree -- simply use #attached on the initial $form['table']

Aside from that, I'm not 100% sure that all of the non-admin CSS belongs into .theme (i.e., with no or nothing in .base) -- does the output still "work" in Stark after manually removing book.theme.css from the output?

23 days to next Drupal core point release.

the output still works in stark after killing the book.theme.css - ive added 2 screenshots
im pretty sure we should fix the navigation class names so they got unified all over drupal, but that thats another issue though.

didnt get the new files ...

lets try with the need review added ..

can someone tell my why the testbot gimme:

error: modules/book/book.admin.css: No such file or directory
error: modules/book/book.theme-rtl.css: No such file or directory
error: modules/book/book.theme.css: No such file or directory].

now that they are defined in the patch

index e69de29..1ec4614 100644
--- a/modules/book/book.admin.css
+++ b/modules/book/book.admin.css
@@ -0,0 +1,12 @@
+#book-outline {
+ min-width: 100em;
+}
+.js #edit-book-pick-book {
+ display: none;
+}
+.form-item-book-bid .description {
+ clear: both;
+}
+#book-admin-edit .form-item {
+ float: left;
+}

@mortendk, that patch looks whack. If you upload a zip of the book module directory, I can reroll a patch for you...

@morten: Your Git diff is formatted in such a way that a book.admin.css must exist prior to patching rather than creating a new file.

alright one more time for prince knud

Looks pretty much RTBC after changing that drupal_add_css() call into #attached

needs alphabetized properties :)
http://drupal.org/node/302199

Re-rolled #30 with alphabetized properties. I also checked all the modified CSS against the CSS coding standards, and found no other problems.

Disregard #32, I accidentally left the newly defined .css files out of the patch.

Attached is the proper patch that corresponds to #30 but with alphabetized properties.

Subscribe. Hope to see this get into D8 -> #736604: book module now without clearfix & better markup + accessibility love without bumping into this cleanup initiative.

Rerolled patch to account for changes committed in #120368: Book outline page breaks my site layout

@mgifford: Can you review this?

I tested this against core. It looks good. Thanks for keeping up with core!

Updated the issue summary to include full information about what "clean up" means.

Just tested this patch and here is my feedback, and a new patch reflecting that feedback:

- The rules below in admin.css looks like it belongs in base.css. This is module behavior and not styling. Also, these rules apply to the node edit form, and admin.css was not being loaded there. I've moved them to base.css and added base.css to the form alter for the node edit form.

.js #edit-book-pick-book {
  display: none;
}
.form-item-book-bid .description {
  clear: both;
}

- The last css rule in admin.css adds a useless float, since that form-item is nicely handled by the draggable table layout.

#book-admin-edit .form-item {
  float: left;
}

- Since (re)moving these rules empties the admin.css, I've removed it entirely.

New patch attached!

Added @file and block level comments as requested in another thread.

Thanks for 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). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

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

Oops, forgot to set status.

Rerolled the patch for the new folder structure.

put the book navigation into

+++ b/core/modules/book/book-navigation.tpl.phpundefined
@@ -30,22 +30,22 @@
\ No newline at end of file

Note that this patch removes the terminal newline from this file, so we'll need to add that back. (Put the cursor on the last line and hit enter.)

And apparently it also needs some other work. :)

ooh my so i also have to rewrite the .test file ?
- this patch made the navigation have the same markup as the pagers ?

I've done some work on the test for Morten's patch--cleaned up about half the fails so far. I'll post whatever I get done so that it can be reviewed.

Changed the book.test for #43

+++ b/core/modules/book/book-navigation.tpl.phpundefined
@@ -30,22 +30,22 @@
+<?php endif; ?>
\ No newline at end of file

Please add the newline here back (as per #45).

Fix that pesky newline :)

Yay!

Wait, what's going on here?

1. The markup changes need to be removed. That's being worked on #736604: book module now without clearfix & better markup + accessibility love. We can't just hijack that work and bring it over here without even saying anything. This is why I suggested combining the 2 issues, but that would require doing a review and adding the CSS in the other issue.
2. The latest patch doesn't add the book.theme.css or book.base.css contents that Morten added.
3. It's a little odd that book.theme.css is loaded by default and book.base.css is not, but it also seems to make sense.
4. The CSS properties need to be in alphabetical order.

Other than those issues the CSS, and comments look good. I think that the other patch is close. I'm going to go review it now.

For now, let's re-roll with just the CSS changes.

1 well wasn't aware of the work on the markup in another issue
im gonna move it out and post it over at the other issue, even imho i think thats double work .. ooh well.

hmm looks like book.base.css should actually be a book.admin.css instead

Looking at the feedback from Jacine in #53, it feels like the patch in #42 is still correct, right? At least until the other patch get's in, and we can remove some css here.

@mortendk: this looks like a typical piece of CSS for a base.css instead of admin.css? It's basic hide/show behavior, not styling.

.js #edit-book-pick-book {
  display: none;
}

yup but its only effective in the admin part of the module. Its not relevant to the rest of the modules functionality

The idea in .base.css is a crucial element to the overall modules functionality that you will always carry around, so in this case i would argue that its an "admin thing" and not a "overall functionality of the module"

module.admin.css
Should hold styles that are only applicable to administrative pages.

so i think we should add it to the admin.css file & remove the base.css file in the book module.
... even that it could be seen as a little bikeshedding ;)

Ok then, one more attempt from my side :)

1. The editing of content can be done in frontend and backend theme, so the CSS currently in base.css would not be available if a user removes all theme.css files and would remove basic behavior in that way.

2. This is what is said in the issue description:

Should hold structural and behavior related styling

That said: I promise not to kill anyone for putting it in admin.css, so the decision is up to you :)

hmm looks like we have hit the first "is it admin or base" dilemma
in my view its an administration task so it should be in the admin.css - its not crusial to the functionality of the module (aka it dosnt break anything if we leave it out) it just looks funky in the tab under book options with that big gray button.

I have rerolled it & killed the ID in the book-navigation.tpl - guess thats okay- even that we have the other issue about the general markup ?

could somebody else weight in here, so we can get a good concensus about the BAT files ?

im an idiot this is the patch rerolled with the base.css -> admin.css renaming.
no markup changes etc. that we can take on later ? when this one is in ...

...and this time with review status #facepalm

Regarding #58, I had similar doubts about a .admin.css in another issue. Is there a meta discussion somewhere for that point?

#60 looks pretty good, outside of that question. (I had no idea what to make of #43 but I sure knew it shouldn't take away our terminal newline.) ;)

An aside: When you mark an issue needs review, it tests all previously untested patches. So if you forget when you upload the patch, no need to upload it a second time--just set the status to needs review in a new comment and testbot will pick it up.

@xjm, have a look at this CSS Cleanup document that explains the rationale behind .admin.css files

http://drupal.org/node/1089868

Alright... So, I am going to agree with @morten on this one. Sorry @jyve!

You've both got good points, but the main focus is front-end facing code. We could eventually start doing module.admin.base.css and module.admin.theme.css but that'd be a little out of hand right now. That's why there's only one-bucket for administrative CSS right now. Putting that code in .base.css would result in a wasted http request for code that's not going to be needed 80% of the time, so my vote is for .admin.css.

@xjm There's a bunch of discussion on #885228: CSS Files are in major need of clean up! and #777814: Allow modules to provide styles for themes (finally leverage structure/behavior split of CSS files). :)

now that we have the theme.css file i have added the styles to bartiks style.css
screenshots added for both bartik & stark without theme.css

Ok so looks like I lost this one :)

Just looked at your last patch @mortendk, and I wonder why you duplicate the book.theme.css into Bartik? I don't see how this is usefull since it would make Bartik really bloated, or am I missing something here?

maybe its a brainfart from my side but shoudnt bartik work & look right without book.theme.css ?
... yeah guess it gets even more bloated ;)

im pretty sure were gonna stomp into more files where we cant figure out if they are base.css or admin.css

jyve you absolutely right about the bartik css work unessesary bloat offcourse the theme.css will be used when people are using bartik *doh*

here we go its rerolled + i matched the colors so itsnt that ugly gray but the warmer that bartik are using allready. (if thats gonna keep this away from rtbc im gonna scream)

Tested the patch in #67, and I think we finally got it right!

Patch looks perfect to me.

ill set it to rtbc ...and do a little happy dance :)

You can't RTBC your own patch. :P

i know xjm ;) but was hoping for a silent moment while i was dancing - can you review it

Im moving the patch over to #736604: book module now without clearfix & better markup + accessibility love and marks this as a duplicate so we can roll the 2 of them together.

Add CSS Cleanup docs.