It's highly annoying that there are colours specified inside admin.css.
I can live with text-sizes, padding/margins, and all other things we would like to put it in by default, but not this..

Simple and straight forward patch. 1 review should be enough to set this RTBC..

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m3avrck’s picture

Status: Needs review » Closed (duplicate)

This went in yesterday in a more robust fashion: http://drupal.org/node/80717

m3avrck’s picture

Status: Closed (duplicate) » Needs work

Oops, was thinking of the other colors, looks like a few were missed.

If you're going to remove colors from admin.css, then you need to update all of the themes drupal ships with some default colors.

Stefan Nagtegaal’s picture

Status: Needs work » Needs review

I did not put extra definition inside our themes, because I think the pages look cleaner without extra styling.
Besides that, maybe we can get rid of all the themes we currently have in core and finally put some new (decent) themes in it?

eaton’s picture

I did not put extra definition inside our themes, because I think the pages look cleaner without extra styling.

A big -1 to that. I'm in favor of moving the definitions to the themes, but taking them out entirely is not a 'bug fix' -- it's a debate about aesthetics.

Besides that, maybe we can get rid of all the themes we currently have in core and finally put some new (decent) themes in it?

That has nothing to do with CSS being in the theme vs. the defaults. :)

m3avrck’s picture

Assigned: Stefan Nagtegaal » m3avrck
FileSize
2.73 KB

Here's a better patch. This moves those hardcoded colors into theme specific styles, this follows the last admin css patch that went in. admin.css should be fully santized now :-)

Stefan Nagtegaal’s picture

Status: Needs review » Reviewed & tested by the community

Works as expected, and is indeed a somewhat better approach..
Setting RTBC..

(This does not say, I don't think we need new themes in core)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)