Here we go again: overly specific CSS

[Antechinus]

Shades of 2.0.x.

Ok you lot, why for you have stuff like this?

Code Select Expand

.progress_bar div.full_bar {
padding-top: 1pt;
width: 100%;
color: black;
position: absolute;
text-align: center;
font-weight: bold;
border-radius: 3px;
z-index: 2;
}
.progress_bar div.green_percent {
height: 15pt;
background-color: #c1ffc1;
background-image: linear-gradient(to bottom, #c1ffc1, green);
border-radius: 3px;
z-index: 1;
}
.progress_bar div.blue_percent {
height: 15pt;
background-color: #98b8f4;
background-image: linear-gradient(to bottom, #98b8f4, blue);
border-radius: 3px;
z-index: 1;
}

Relevant points are:

1/ Are the full_bar, green_percent and blue_percent classes used anywhere on HTML tags that are not divs? Not as far as I can see.

2/ Ok, so if not, why are you declaring them as div.full_bar, etc? Why not just use the class name? It works, it's less code, it's more efficient, and the lower degree of specificity will make customisation easier (trust me, it will).

3/ Also, while we're on the subject, are the green_percent and blue_percent classes used anywhere except inside .full_bar? Not as far as I can see, in which case there is no need to declare them as descendants. Just use the class name. That's what it's there for. Again, it works, it's less code, it's more efficient, and the lower degree of specificity will make customisation easier.

4/ Ok, so what if someone decides they don't want their "green_percent" bits green? What if they want them purple? You now have a class name which makes absolutely no sense. A fundamental principle of good markup is "use class names that are still usefully descriptive even if someone changes the presentation".

5/ Why specify z-index: 1;? Unless I'm missing something, this should not be required. It's default behaviour. Has it been tested without that declaration to see if it is actually required?

[Antechinus]

Oh goody. We haz moar.

Code Select Expand

/* Styles for the manage calendar section. */
dl.settings dt.small_caption {
width: 20%;
}
dl.settings dd.small_caption {
width: 79%;
}

/* Styles for the manage search section. */
dl.settings dt.large_caption {
width: 70%;
}
dl.settings dd.large_caption {
width: 29%;
}

K, so these are only used in a couple of places, so again they do not need to be specified as descendants. They do require the tag names, but only because the same class name has been used for both dd and dt. If the dd and dt were given different class names you could drop the tag names from the css.

So this is perfectly adequate with the current markup:

Code Select Expand

/* Styles for the manage calendar section. */
dt.small_caption {
width: 20%;
}
dd.small_caption {
width: 79%;
}

/* Styles for the manage search section. */
dt.large_caption {
width: 70%;
}
dd.large_caption {
width: 29%;
}


By changing the class names to make them more descriptive you could further simplify the CSS ("simplify" in the sense of using a more efficient selector).

Code Select Expand

/* Styles for the manage calendar section. */
.dt_small_caption {
width: 20%;
}
.dd_small_caption {
width: 79%;
}

/* Styles for the manage search section. */
.dt_large_caption {
width: 70%;
}
.dd_large_caption {
width: 29%;
}

[Arantor]

Don't you mean:

Code Select Expand

/* Styles for the manage calendar section. */
.dt_small_caption {
width: 20%;
}
.dd_small_caption {
width: 79%;
}

/* Styles for the manage search section. */
.dt_large_caption {
width: 70%;
}
.dd_large_caption {
width: 29%;
}

[Antechinus]

Beat you to it.

[Arantor]

I should damn well hope so, CSS god

[Antechinus]

Anyway, we haz moar.

Code Select Expand

ul.pending_payments {
margin: 0;
padding: 0;
}
ul.pending_payments li {
list-style-type: none;
}


Y'know, ever since 2.0.x dev days there has been this in index.css:

Code Select Expand

/* A quick reset list class. */
ul.reset, ul.reset li {
padding: 0;
margin: 0;
list-style: none;
}

So, why have extra CSS for .pending payments?  In fact, unless it's required for js somewhere, why have .pending_payments at all? The boring old reset class would do the same job if declared in the markup, with less CSS overall.

[Antechinus]

Code Select Expand

table.table_grid.perm_grid {
margin: 0.2em;
width: 49%;
}

Why for needz tag name? Why notz can be done with two chained classes?

[Arantor]

Github is ----> that way

[Antechinus]

Oh goody, lookit dis:

Code Select Expand

dl.admin_permissions dt {
width: 35%;
}
dl.admin_permissions dd {
width: 64%;
}

The admin_permissions class is only ever used for a dl, therefore tag name not required.

Code Select Expand

.permission_groups {
padding: 0;
margin: 0;
}
.permission_groups li {
list-style-type: none;
padding: 0.2em;
margin: 1px;
}
.perms {
width: 20px;
display: inline-block;
text-align: center;
}

/* Styles for the themes section. */
ul.theme_options {
padding: 0;
margin: 0;
}
ul.theme_options li {
list-style: none;
padding: 0.4em;
}

These ul's might as well be declared as .reset and save a pile of CSS.

Don't need tag names for the parents here:

Code Select Expand

dl.themes_list {
margin: 0;
}
dl.themes_list dt {
margin-bottom: 3px;
}
dl.themes_list dd {
font-style: italic;
white-space: nowrap;
}

[Antechinus]

Github is ----> that way

Wot iz this GitHub you speak of?

[Arantor]

https://github.com/SimpleMachines/SMF2.1

[Antechinus]

Oh and there shouldn't be any need to use stuff like this anywhere:    margin: 0 !important;

It's not important! If someone wants a 53px margin there, they can have it. If you want 0 margin in default, just use 0 margin.

Primer: http://www.smashingmagazine.com/2010/11/02/the-important-css-declaration-how-and-when-to-use-it/

[Antechinus]

https://github.com/SimpleMachines/SMF2.1

I hate GitHub. Seriously hatez it. I am not, repeat not, doing pull requests.

[Kindred]

The important is actually needed in several cases...

[Antechinus]

Shouldn't be. That implies a cascade balls-up somewhere.

Anyway:

Code Select Expand

/* Those collapse and expand icons are wonderful. */
.toggle_up:before, .toggle_down:before {
width: 17px;
height: 17px;
display: inline-block;
background: url(../images/toggle.png) no-repeat 0 0;
text-indent: -100em;
overflow: hidden;
content: '';
vertical-align: middle;
margin: -5px 5px 0 5px;
}

Why have a huge negative text-indent when there is no text at all in the pseudo element? Also, it probably doesn't need the vertical-align if using margins to correct vertical positioning. Might as well skip the align and just play with the margin.


ETA: Line height for standard body text is already declared in the body tag's font declaration. No need to repeat it here. Not really sure 1px top and bottom padding is useful here either.

Code Select Expand

/* Posts and personal messages displayed throughout the forum. */
.post, .personalmessage {
overflow: auto;
line-height: 1.4em;
padding: 1px 0;
}


Inconsistent color declarations. Better to keep them all hex, if not requiring alpha channel.

Code Select Expand

/* Calendar colors for birthdays, events and holidays */
.birthday {
color: #920ac4;
}

.event {
color: #078907;
}

.holiday > span {
color: rgb(0, 92, 255);
}


More redundant tag names:

Code Select Expand

dl.settings {
clear: right;
overflow: auto;
margin: 0 0 10px 0;
padding: 5px;
}
dl.settings dt {
width: 40%;
float: left;
margin: 0 0 10px 0;
padding: 0;
clear: both;
}
dl.settings dt.settings_title {
width: 100%;
float: none;
margin: 0 0 10px 0;
padding: 5px 0 0 0;
font-weight: bold;
clear: both;
}
dl.settings dt.windowbg {
width: 98%;
float: left;
margin: 0 0 3px 0;
padding: 0 0 5px 0;
clear: both;
}
dl.settings dd {
width: 56%;
float: right;
overflow: auto;
margin: 0 0 3px 0;
padding: 0;
}
dl.settings img {
margin: 0 10px 0 0;
vertical-align: middle;
}
/* help icons */
dl.settings dt a img {
position: relative;
vertical-align: top;
}

/* a general table class */
table.table_grid {
border-collapse: collapse;
margin: 1px 0 5px 0;
width: 100%;
}
table.table_grid td {
padding: 3px;
}


Shouldn't need to declare li here:

Code Select Expand

/* For cases where we want to spotlight something specific to an item, e.g. an amount */
.dropmenu li .amt, #top_info li .amt {
margin-left: 3px;
padding: 0 5px;
color: white;
background: #6d90ad;
border-radius: 8px;
}
.dropmenu li .active .amt, #top_info li .active .amt {
background: none;
color: inherit;
}

Come to think of it, probably don't need to declare .dropmenu there either (would need to test).

[Arantor]

Because when I originally wrote it (which didn't use :before, I'll note), I did have it supporting text inside the element for navigational elements with textual content for speech readers.

[Antechinus]

Ah, ok. That makes sense. Probably not a bad idea to have the text and the indent. Although you could just use 100% instead. Saves a little bit of browser work since the boxes it has to draw are smaller, and the hidden overflow will still work fine.

Anyway I'll get bored soon but here's another one:

Code Select Expand


.dropmenu li a, #top_info > li > a {
padding: 0 7px 0 7px;
margin: 0;
display: block;
border: 1px solid transparent;
border-radius: 4px;
}
/* Level 1 active button. */
.dropmenu li a.active, #top_info li a.active {
background: orange;
color: #fff;
font-weight: bold;
border: 1px solid #f49a3a;
box-shadow: 0 5px 5px rgba(255,255,255,0.2) inset;
text-shadow: 1px 1px 3px rgba(0, 0, 0, 0.6);
}

Shouldn't need the li tag at all. Most likely can get away without the a tag in the second declaration as well, and just use the class name. At worst, a.active should be adequate. Zero margin shouldn't be required, offhand. Can't see why it would be.

[Arantor]

Can't just use a.active in the latter one because that would end up targeting buttons in the button strips (e.g. the reply button in reply/add poll/mark unread) which is also a.active.

I would agree with you about going for .dropmenu a.active, #top_info a.active though.

Don't forget there are some interesting weird things in the top info area done with AJAX (which are mostly my fault)

[Antechinus]

No I meant dropping the li but keeping the .dropmenu and #top_info. Should get away with .dropmenu .active AFAICT at the moment.

The overly specific stuff in 2.0.x is a right pain sometimes. Best to keep it as low as possible.

[Bloc]

The overly specific stuff in 2.0.x is a right pain sometimes. Best to keep it as low as possible.

"Overly" is just the start lol. It should have rewritten from grounds up, with better html too. Not a task for the faint of heart though.