In the default node.tpl.php in node.module, we have print $terms . Because $terms is a list, this is invalid html markup (lists are not allowed inside spans). This was the only w3c html validation error I got with drupal 6. I propose changing the span to a div and setting the div to display: inline. This shuts up the validator and looks the same.

CommentFileSizeAuthor
#10 terms_3.patch971 bytesjody lynn
#2 terms.patch951 bytesjody lynn
terms.patch952 bytesjody lynn

Comments

jody lynn’s picture

jody lynn
she/her
Primary language English
commented

sorry, that code was supposed to read ''$terms'', but is getting altered

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
StatusFileSize
new terms.patch951 bytes

lol. Take 3, this time with code tags: <span class="terms"><?php print $terms ?></span> Also noticed an oversight in my previous patch

dvessel’s picture

dvessel commented
Status: Needs review » Reviewed & tested by the community

Nice catch. :)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

.terms sounds like a pretty too broad selector to add now. What about making it more specific, so we are not breaking stuff elsewhere?

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

I disagree Gabor. The .terms selector was already the one being used. The only way this change will affect people will be if their themes were using a span.terms selector, since if they were just using .terms it will have the same behavior now. Changing it to something else will do more harm than good since existing stylesheets would need to be changed.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

Well, I cannot see where was the .term selector used except in Garland:

gabor$ grep -r "\.terms" *
themes/garland/style-rtl.css:.terms ul.links li {
themes/garland/style-rtl.css:.terms {
themes/garland/style.css:.terms ul.links li {
themes/garland/style.css:.terms {

So if someone has a theme which has a class="terms" outside of the node terms list (eg. in a sidebar or whatever), it is affected by this change, since you are introducing the first selector on this in a core module. All stuff with class="terms" will be display:inline, while before this patch, nothing applied to class="terms" if you were not using Garland.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented

Ok, so how about if it becomes something like <div class="terms terms-inline"> and then .terms-inline {display: inline; } so that .terms still applies as it used to.

dvessel’s picture

dvessel commented

Using a more specific selector does make sense but that then depends on the theme. The markup in the default node.tpl.php isn't even used by default but do we use a selector specific to that anyway? I'm not sure. Would 'div.terms' be specific enough?

edit: Lynn, that could work too.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Well, the code in question is right in node.tpl.php anyway, so it is dependent on a theme as well. .terms-inline could also work.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new terms_3.patch971 bytes

Here's a patch with the .terms-inline selector.

dvessel’s picture

dvessel commented
Status: Needs review » Reviewed & tested by the community

Tested on a CSS based theme.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.