2.1 BoardIndex.template.php: redundant markup.

Started by Antechinus, September 08, 2015, 04:48:19 AM

Previous topic - Next topic

Antechinus

Well, you did set up a Bug Reports board. It'd be a pity to waste it.

Code (BoardIndex.template.php) Select
<div class="icon">
<a href="', ($board['is_redirect'] || $context['user']['is_guest'] ? $board['href'] : $scripturl . '?action=unread;board=' . $board['id'] . '.0;children'), '">
<span class="board_', $board['board_class'], '"', !empty($board['board_tooltip']) ? ' title="' . $board['board_tooltip'] . '"' : '', '></span>
</a>
</div>


Firs thing that strikes me here is that 2.1 has too many different elements doing different things, and all given class="icon". IMO it would make more sense to give them more descriptive class names. In this case I think class="board_icon" would be a lot better. It will save confusion down the track.

Next thing I think is that there's no need to have a span in there. It doesn't do anything useful. You already have two elements to play with (div and anchor) and they are more than adequate for what you need to do. So you could easily shorten the above to this:

Code (BoardIndex.template.php) Select
<div class="board_icon">
<a href="', ($board['is_redirect'] || $context['user']['is_guest'] ? $board['href'] : $scripturl . '?action=unread;board=' . $board['id'] . '.0;children'), '"  class="board_', $board['board_class'], '"', !empty($board['board_tooltip']) ? ' title="' . $board['board_tooltip'] . '"' : '', '></a>
</div>


The CSS for it would need tweaking, but it could do with some tweaking anyway. It currently looks like this:

Code (makebabyjesuscry.css) Select
div.icon span {
background: url(../images/boardicons.png) no-repeat 0 0;
display: inline-block;
width: 45px;
height: 45px;
}
div.icon span.board_on {
background-position: 0 0;
}
div.icon span.board_on2 {
background-position: -45px 0;
}
div.icon span.board_off {
background-position: 0 -45px;
}
div.icon span.board_redirect {
background-position: -45px -45px;
}
div.icon {
float: left;
width: 5%;
text-align: center;
padding: 8px 0 0 0;
min-width: 60px;
}


First question: is there any other place in 2.1 where a span is placed inside any element that has class="icon"? If not, there is no need to declare the tag name here:

Code (makebabyjesuscry.css) Select
div.icon span {
background: url(../images/boardicons.png) no-repeat 0 0;
display: inline-block;
width: 45px;
height: 45px;
}


It'd be better (really, for several reasons) to just use .icon span here. You can also save some bytes by using display: block; since there's no requirement to use inline-block; in this situation. The span is not inline with anything else.

OTOH, if you change the class name to the more descriptive .board_icon and make the other suggested markup changes it gets even better. What's there now can be replaced with this:

.board_icon {
float: left;
width: 5%;
text-align: center;
padding: 8px 0 0 0;
min-width: 60px;
}
.board_icon>a {
background: url(../images/boardicons.png) no-repeat 0 0;
display: block;
width: 45px;
height: 45px;
}
.board_on2 {
background-position: -45px 0;
}
.board_off {
background-position: 0 -45px;
}
.board_redirect {
background-position: -45px -45px;
}


This is shorter, easier to read, and easier to comprehend, and it uses more efficient selectors. Selecting by both tag name and class name isn't a good way to do CSS and should be avoided if possible. If you have to use a tag name and a class name in CSS, it usually means you've made a mistake somewhere.

You could also simplify the sprite a bit by having all the icons either horizontal or vertical. There's no need to use a square image. It won't improve performance or make customisation easier. OTOH, if the adjustments for different classes only have to change positioning on one axis, this will be easier for average people to get their heads around.

So if you put them all horizontal you could use this:


.board_on2 {
background-position: -45px 0;
}
.board_off {
background-position: -90px 0;
}
.board_redirect {
background-position: -135px 0;
}


Note that in both cases there is no need to declare .board_on because you have already declared exactly the same positioning for .board_icon>a and that will take care of .board_on automatically.


Once again, I can think of more details too, if anyone is interested.

Shambles

Are your suggestions for improvement actually bugs, though?

Antechinus

Well they don't have a board called "2.1 suggestions for improvements". It depends how you look at it. If it can easily be made better than it is, is this a bug fix or not? Even if you want to argue about that, there's still another question to answer. If it can easily be made better than it is, should it be made better, or should it be left worse? Take your pick.

I'm deliberately restricting myself to suggestions that are easy to implement, have clear benefits, and won't turn Beta 2 upside down.

Oldiesmann

That same code is also used in MessageIndex.template.php, but I can't think of anywhere else beyond that where it would be used.
Michael Eshom
Christian Metal Fans

Antechinus

#4
Here's another thought. This one really is simple. It doesn't even require any CSS by default.

There's a discussion in the SMF Documentation Help board about putting images before, or after, the board name or board description: http://www.simplemachines.org/community/index.php?topic=539423.0

This is apparently asked about frequently enough for it to be worth its own Wiki page, so it's probably something you should make easier if you can. With HTML being disabled there in 2.1 it has to be done with CSS, which is easy enough for the board name.

It can still be done for the board description, but at the moment that gets a bit convoluted. You have to use descendant selectors and nth-child to do the job.

Code (example.css) Select
#board_1 .info p:nth-of-type(1):before {stuff goes here}

If you change the markup in BoardIndex.template and MessageIndex.template to give the board description paragraph a class name, this greatly simplifies the CSS.

Code (markup) Select
echo '

<p class="board_description">', $board['description'] , '</p>';


Code (example.css) Select
#board_1 .board_description:before {stuff goes here}

That's definitely going to be less brain frazzling for most people, as well as being more efficient. :)

Antes


Antechinus

Yeah looks pretty good. It's still overly specific on the CSS though. You don't need to use the div tag name so much. Just the class name is enough to do the job in 90% of cases.

Take .board_icon. It's only ever a div, right? So just call .board_icon and save yourself some code. Shorter file, more efficient selector, faster processing and easier customisation. It's pure win all the way :)

And you missed this. ;)

QuoteNote that in both cases there is no need to declare .board_on because you have already declared exactly the same positioning for .board_icon>a and that will take care of .board_on automatically.

This should work equally well.

/* BoardIndex */
/* This place covers board places (boardindex-messageindex) */
h3 .collapse {
float: right;
margin: 4px 4px 0 0;
}
.board_icon a {
background: url(../images/boardicons.png) no-repeat 0 0;
display: inline-block;
width: 45px;
height: 45px;
}
.board_on2 {
background-position: -45px 0;
}
.board_off {
background-position: 0 -45px;
}
.board_redirect {
background-position: -45px -45px;
}
.board_icon {
width: 5%;
text-align: center;
padding: 8px 0 0 0;
min-width: 60px;
}
.info {
width: 50%;
margin: 10px 0 5px 0;
}
.info .subject {
font-weight: 600;
font-size: 1.1em;
color: #A85400;
}
.stats {
width: 14%;
padding: 10px;
font-size: 0.9em;
}
.stats p {
border-left: 1px solid #DDD;
border-right: 1px solid #DDD;
text-align: center;
min-height: 3em;
margin: 3px 0 0;
}
.lastpost {
width: 25%;
font-size: 0.9em;
}
.lastpost p {
margin: 3px 0 0 0;
}
.board_icon, .info, .stats, .lastpost {
min-height: 4.5em;
display: table-cell;
vertical-align: middle;
}
.main_container {
margin-bottom: 20px;
}
.up_contain {
overflow: hidden;
box-sizing: border-box;
border: 1px solid #DDD;
margin-bottom: 3px;
margin-top: -1px;
}
/* Child boards */
.children {
width: 100%;
float: left;
border-top: 1px solid #DDD;
padding: 5px;
}
.children p {
font-size: 0.9em;
}
.moderators {
font-size: 0.9em;
font-weight: bold;
}
.postby {
width: 100%;
float: left;
}


I'll test that live on my local since I have it running at the moment.

Antes

oh my bad, after 30 hours long trip with awful bus... 0 0 already means nothing...

btw, you need the "board_icon a" part because you are loading the sprite into "board_icon a". Otherwise css looks for unknown location's position.

Antechinus

Yeah you do need the tag name there. I agree that's one of the exceptions to the general rule. :)

I just had a play with it on local. This CSS seems fine, although I haven't set up moderators for boards yet. The rest of the index seems to be cool.

I found I was slightly too optimistic on the background positioning for the board icon anchors. If you just keep the same declaration for .board-icon a, that has higher specificity than .board_off etc so overrides them.

What works is to not declare positioning on .board_icon a since it'll default to 0 0 anyway. If you declare just background-image and background-repeat, you can just use class name for the other states and it works.

Also noticed that you missed adding the .board_description class in MessageIndex.template.php. ;)

h3 .collapse {
float: right;
margin: 4px 4px 0 0;
}
.board_icon a {
background-image: url(../images/boardicons.png);
background-repeat: no-repeat;
display: inline-block;
width: 45px;
height: 45px;
}
.board_on2 {
background-position: -45px 0;
}
.board_off {
background-position: 0 -45px;
}
.board_redirect {
background-position: -45px -45px;
}
.board_icon {
width: 5%;
text-align: center;
padding: 8px 0 0 0;
min-width: 60px;
}
.info {
width: 50%;
margin: 10px 0 5px 0;
}
.info .subject {
font-weight: 600;
font-size: 1.1em;
color: #A85400;
}
.stats {
width: 14%;
padding: 10px;
font-size: 0.9em;
}
.stats p {
border-left: 1px solid #DDD;
border-right: 1px solid #DDD;
text-align: center;
min-height: 3em;
margin: 3px 0 0;
}
.lastpost {
width: 25%;
font-size: 0.9em;
}
.lastpost p {
margin: 3px 0 0 0;
}
.board_icon, .info, .stats, .lastpost {
min-height: 4.5em;
display: table-cell;
vertical-align: middle;
}
.main_container {
margin-bottom: 20px;
}
.up_contain {
overflow: hidden;
box-sizing: border-box;
border: 1px solid #DDD;
margin-bottom: 3px;
margin-top: -1px;
}
/* Child boards */
.children {
width: 100%;
float: left;
border-top: 1px solid #DDD;
padding: 5px;
}
.children p {
font-size: 0.9em;
}
.moderators {
font-size: 0.9em;
font-weight: bold;
}
.postby {
width: 100%;
float: left;
}

Antes

I open two SMF 2.1 install side-by-side to see the difference and do some changes in both... You get the rest :D

Antechinus

Oh yeah there's one bit of stray old code in Recent.template.php > function template_unread()

The little "posted" icon is missing the .posted class, and still has the gruesome WIP inline CSS I was frigging around with yonks ago.

Antechinus

Hey I just spotted something else that could be simplified (one of my ancient sins).

.children {
width: 100%;
float: left;
border-top: 1px solid #DDD;
padding: 5px;
}
.children p {
font-size: 0.9em;
}
.moderators {
font-size: 0.9em;
font-weight: bold;
}
.postby {
width: 100%;
float: left;
}


The float: left; width: 100%; is an ancient and brutal way of making a parent element wrap floated content. Since .children doesn't contain any floated content in 2.1, you can ditch that.

Also .postby should be just as good if declared as display: block; as this does the same job anyway and is shorter and more tolerant.

So updated CSS looks like this:

h3 .collapse {
float: right;
margin: 4px 4px 0 0;
}
.board_icon a {
background-image: url(../images/boardicons.png);
background-repeat: no-repeat;
display: inline-block;
width: 45px;
height: 45px;
}
.board_on2 {
background-position: -45px 0;
}
.board_off {
background-position: 0 -45px;
}
.board_redirect {
background-position: -45px -45px;
}
.board_icon {
width: 5%;
text-align: center;
padding: 8px 0 0 0;
min-width: 60px;
}
.info {
width: 50%;
margin: 10px 0 5px 0;
}
.info .subject {
font-weight: 600;
font-size: 1.1em;
color: #A85400;
}
.stats {
width: 14%;
padding: 10px;
font-size: 0.9em;
}
.stats p {
border-left: 1px solid #DDD;
border-right: 1px solid #DDD;
text-align: center;
min-height: 3em;
margin: 3px 0 0;
}
.lastpost {
width: 25%;
font-size: 0.9em;
}
.lastpost p {
margin: 3px 0 0 0;
}
.board_icon, .info, .stats, .lastpost {
min-height: 4.5em;
display: table-cell;
vertical-align: middle;
}
.main_container {
margin-bottom: 20px;
}
.up_contain {
overflow: hidden;
box-sizing: border-box;
border: 1px solid #DDD;
margin-bottom: 3px;
margin-top: -1px;
}
/* Child boards */
.children {
border-top: 1px solid #DDD;
padding: 5px;
}
.children p {
font-size: 0.9em;
}
.moderators {
font-size: 0.9em;
font-weight: bold;
}
.postby {
display: block;
}


Incidentally, anywhere you find float: left; width:100%; declared there's probably a better way of doing it, so if it's being a nuisance it can be replaced with several options depending on the situation (auto overflow, hidden overflow, pseudo element clearfix, etc).

Antechinus

By the way, this part in rtl.css will be deprecated now, so can be removed:

/* This place covers board places (boardindex-messageindex) */
div.icon, div.info, div.stats, div.lastpost {
float: right !important;
}


Antechinus

Cool. Did you catch that you had another .stats in the forum stats .subbg? That broke due to the other changes. Easy fix. How about just changing that span to .forum_stats?

<a href="', $scripturl, '?action=stats" title="', $txt['more_stats'], '"><span class="generic_icons forum_stats"></span> ', $txt['forum_stats'], '</a>

And:

.generic_icons.forum_stats {
background-position: -187px -109px;
}

Antes

stats class are only good with dl/dt/dd other one is the icon there is no conflict i seen so far so no need to change them.

Antechinus

It broke on my local. It was inheriting the table-cell and width from the .stats div in the board rows.

Antes


Antechinus

Oh cool. Yeah that's another way of doing it. No worries.

Antechinus

Mind you, my way was less work. :D Anyway will apply your changes on local. :)

Advertisement: