In noderelationships.pages.inc, on line 203, you use the dl-element (in combination wit dt and dd). This is quite a unusual way of using this kind of markup. The dl-element stands for Defenition List. As the output is not a defenition list, I think some other markup should be used. There is also a practical disadvantage of this element. The standard display gives the dd-element a margin to the left, which in my expercience is rarely appropriate.

I would suggest to use a div as the surrounding element (instead of the dl), use a heading for the title (say a h2 or h3 instead of the dt) and a div for the content.

Also because it is in a function and not a template, to me it is less attractive to override.

Any thoughts on this?

Comments

rolfmeijer’s picture

rolfmeijer commented

The overriding is not really an issue. In the fuction nothing special happens, except adding the markup. So overriding is not much of a problem. (Forget I said it ;-) )

markus_petrux’s picture

markus_petrux commented

I do not exactly recall in which moment of the development of this module this dl element landed here. I probably forgot to revisit this theme function when the development was more advanced. My bad, and you're right that it is not semantically correct. However, I'm afraid to change this now, as we could now break sites where it has been styled using CSS overrides.

As you have noted, you can easily override this theme function. So, I'm tempted to keep things as-is.

rolfmeijer’s picture

rolfmeijer commented
Status: Active » Closed (won't fix)

Yes, I see your point. I’ll start overriding right away! ;-)
Thanks for your quick answer.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Closed (won't fix) » Active

Hmm marking this active as I think it should still be addressed. The default behavior probably shouldn't add dl and dd markup as OP suggested. While it may break CSS overrides on some sites - you can probably just add something into the release notes about it. I don't know many site admins that don't go thru release notes before updating a module!

Just my 2cents.

markus_petrux’s picture

markus_petrux commented
Status: Active » Closed (won't fix)

@bkosborne: please, note you can override the theme function in your system to fit your needs. We should not break existing installations that rely on current default behavior.