Clean up the CSS for Aggregator module

Changing tags and subscribing :) Thanks for getting this started @johnvsc!

I think that all of the css for the aggregator module should be in the aggregator.theme.css file. There are a few styles that appear to be admin only but they still seem to be theme related.

One of the styles I was questioning was the #aggregator .feed-source .feed-icon that floats the rss icon left. This would be considered theme right?

The attached patch moves all styles from aggregator.css to aggregator.theme.css

You still need to edit the info file.

modules\aggregator\aggregator.info
       8  stylesheets[all][] = aggregator.css

Hey guys, it's awesome to see some work going here!! :)

Definitely keep in mind though, that cleaning up the CSS isn't just about moving rules to the appropriate files. We should also be looking to clean it up by:

• Removing styles that make too many assumptions or introduce superflous margins, padding and add things like font settings that are really not necessary, and moving that CSS into the core themes where it makes sense.
• Reducing the specify in the selectors we do keep.
• Use classes instead of ID's in markup and module CSS files where possible, since ID's make it more difficult (specificity-wise) to override.
• Use proper HTML elements in template files instead of using a non-semantic tag and mimicking the styling in CSS.

Ideally this clean up will result in lots of CSS files being completely removed across modules. And this module is probably one of the worst offenders out there, so kudos to you guys for getting started on it. Here are some suggestions for moving it forward:

• Change the ID in the aggregator-wrapper.tpl.php to a class, and update the associated CSS.
• Separate the code into chunks with brief comments that describe what the code is for. A lot of this code is here is just thrown together and it's hard to tell what exactly it's for by scanning the file. We should fix that.
• Make sure there are no administrative styles lurking in here. It seems like there might be.
• Test with Stark, and attempt to remove as much CSS as possible before there is a noticeable regression with how it looks. If you decide things like colors or font-size changes need to stay, then you probably want to put those in the core theme CSS files themselves.
• If there's anything left at that point, it's probably safe to say that the code belongs in .theme.css.

Thanks for the quick feedback, I didn't realize we were looking to go that far, but I think it's great. There are definitely some styles that can be removed completely. Am I right in thinking that most of the margin, padding, font size, etc declarations should be left to the theme?

@Jacine, that's a great list to use going forward. Thanks!

@tars16 you're welcome and yes, you're right.

The goal is to have a .base.css file that has stuff that is absolutely necessary for the module to function, and .theme.css that holds a default implementation with as few superflous assumptions as possible instead of having to have that code duplicated in every single core theme (because that doesn't make sense from a maintenance standpoint). Some margins and padding might need to remain here... We don't want to make it so themers need to style random stuff for no reason.

It's definitely subjective and will come down to looking closely at each case, but don't be afraid to modify markup as part of it and push the limits trying to make it as clean as possible. If we get to the point where people will not complain or be bothered by core CSS, that'll be a huge win for all of us.

Have fun ripping it to shreds. :D

EDIT: And of course, once this is done, if you are creating a non-admin theme, you'll be able to use hook_css_alter() to remove all .admin.css files in one shot. This way, the front-end of the site will never include administrative CSS styles.

Looking at the css again it really looks like the majority of the css can be removed. I don't think any of the css is really necessary for the module to function. The majority of the css was formatting the feed items in the feed listings. Looking at the way stark handles the front page node listings (no margins,padding, etc defined for things like taxonomy terms, headings, etc) it makes sense to remove those rules.

I kept the styles for the feed icon being floated right and stuck those in the .theme.css file.

I made the change to the template file that was suggested. Based on the feedback/instructions above I think I'm on the right track. Do you think it's worth keeping font-size and margin declarations in the .theme.css? I feel like all of this should be handled by the theme. If this looks good, I'll get started on some of the other modules.

AWESOME! That sounds great to me. :)

+++ b/modules/aggregator/aggregator.theme.cssundefined
@@ -0,0 +1,8 @@
+
+.aggregator .feed-source .feed-icon {
+  float: right; /* LTR */
+  display: block;
+}
+.aggregator .feed-source .feed-image img {
+  margin-bottom: 0.75em;
+}

1. There shouldn't be an empty line at the top of the file, but there should be a file block. See system.theme.css for an example.
2. We need a RTL version of this that floats the image left. It should be named aggregator.theme-rtl.css.
3. Can we reduce the specificity of this selector?
4. Can we add a comment about what this code is for. There's examples in system.theme.css for this too.
5. This shouldn't happen. Add an empty line to the bottom of the file to prevent this.
   

   \ No newline at end of file
   

Also, screenshots of what this looks like in all the themes would really, really help speed up the review process. 1 before screenshot is enough, but we've also got to make sure we don't introduce any cross-browser issues. I know this is a pain, but it's common practice here for these types of patches. We don't need to support IE6 (the announcement that we're dropping it is coming), but we need IE7-9 and then the same that jQuery supports: http://docs.jquery.com/Browser_Compatibility.

I haven't checked yet, but any reviewers can help with the screenshots, and also check if there is any code in the theme CSS files related to this that needs updating.

Great work so far!!!

Powered by Dreditor.

Thanks!

1. Looks like system.theme.css has a blank line at the top of the file?
2. Added a RTL version
3. Sure can, removed the middle selector
4. Added comments.
5. Added file line at the end of the file

I took screenshots in the browsers I have installed. (FF5, Chrome beta-m, IE7-9) Added them as a .tgz file. Is it easier if they are uploaded as individual jpegs? Did I take screens of what you were looking for? Did not include the before screenshot.

Looks like Garland could use some margin on the h3 tag.

Don't mind the choice of feed, it's the first one I could find with a feed image.

@tars16 Yes, yes, yes!! Great work! This makes me so happy. LOL. :)

There's no reason for the space at the top of system.theme.css, but that's a teeny tiny nitpick. I'm also wondering if the @file block is a waste or not, given the naming scheme of these files. What do you think?

This is really great work, and it's getting very close. Looking at the screenshots, the only thing that needs work is Garland. There's no margin on the headings, so maybe it'd be good to add some top margin to the wrapper so there is some separation between the posts. I'll also do a more thorough review tomorrow, and make sure the related administrative screens and the posts themselves are still all good.

Garland will be removed so why bothering fixing garland now?
I also would remove the @file blocks.

Nice screesnhots!

Thanks.

I agree that the @file blocks are a little pointless since the file names are so specific.

If Garland is being removed, I agree that we shouldn't add anything to fix since it would need to be removed as soon as Garland is removed. I can add the fix if we really want to for the time being.

Ok, then let's remove the file blocks. The Garland fix needs to happen though. People are trying to remove Garland from core, but until/unless that actually happens, we cannot trash it. Basically we cannot break things in any of these patches. Still hoping to find the time to do a thorough test of this later today. :)

Removed the file blocks and added margin-bottom to the feed-item. I chose 20px because that was the largest amount of margin that I could add before it started to effect stark. I prefer to do it at the bottom rather than the top. Thoughts? I attached a new screenshot of Garland in Firefox.

Any test results?

Sorry for taking to long to get around to this. I did some testing and reviewed the patch tonight and here's what I found:

+++ b/modules/aggregator/aggregator.theme-rtl.cssundefined
@@ -0,0 +1,3 @@
+#aggregator .feed-source .feed-icon {

This should be .aggregator .feed-icon, just like in aggregator.theme.css.

+++ b/modules/aggregator/aggregator.theme.cssundefined
@@ -0,0 +1,10 @@
+.aggregator .feed-item {
+  margin-bottom:20px;

This shouldn't be in the module's CSS file, and given what I found in Garland's style.css file (see below), this can be removed.

+++ b/modules/aggregator/aggregator.theme.cssundefined
@@ -0,0 +1,10 @@
\ No newline at end of file

There should be an empty line at the bottom of the file.

A quick look through Garland revealed the following In Garland's style.css:

/**
 * Aggregator.module
 */
#aggregator {
  margin-top: 1em;
}
#aggregator .feed-item-title {
  font-size: 160%;
  line-height: 130%;
}
#aggregator .feed-item {
  border-bottom: 1px solid #e9eff3;
  margin: -1.5em -31px 1.75em;
  padding: 1.5em 31px;
}
#aggregator .feed-item-categories {
  font-size: 0.92em;
}
#aggregator .feed-item-meta {
  font-size: 0.92em;
  color: #898989;
}

This needs to be addressed. Changing all the #aggregator instances to .aggregator does the job.

Other than that stuff, it's looking good. It's great that we'll be able to shed all this CSS! :)

Ok, this should address those issues.

+++ b/modules/aggregator/aggregator.theme-rtl.cssundefined
@@ -0,0 +1,3 @@
+.aggregator .feed-source .feed-icon {

This is the only remaining issue. Change that to .aggregator .feed-icon and this is RTBC. :D

get it committed

Great. Thanks for the reroll Aron, and congrats to you @tars16 for your first core patch!

:D

Committed to 8.x. Thanks.

Hey Dries, the new files didn't make it into the commit. Here's a patch for those.

Err, use this one, without the blank lines.

Looks good.

Aggregator CSS broken qualifies at least as major - as long as it's in core.

Added them to Git. Sorry about that.

Thanks! :D

Cleaning up tags/component.