There were some <br /> and <p> tags in content_profile.module (called from hook_user, not in any theme function) that i have been repeatedly unsetting in my own module.

This patch eliminates them entirely.

I did not move them to a theme function because they don't appear to serve any functional purpose.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AaronBauman’s picture

Status: Reviewed & tested by the community » Needs review

oops - wrong status on that one.

fago’s picture

Does the default template still work fine without those html tags? Any reviewers?

gilgabar’s picture

Status: Needs review » Reviewed & tested by the community

Patch works perfectly for me.

xjm’s picture

Patch works for me; thank you.

fago’s picture

Status: Reviewed & tested by the community » Needs work

I just gave it a test and without the
there is no spacing between the "add profile link" and the next heading. Also I dislike removing the parent "#content-profile-view" element as this might break existing CSS rules of people. Thus better move it into a separate theming function or similar.

AaronBauman’s picture

Assigned: AaronBauman » Unassigned
gilgabar’s picture

It's really poor practice to use br tags for spacing. That should always be handled with CSS. It is easy to use CSS to replicate the spacing provided by the br tag. It is nearly impossible to remove the spacing created by the br tag with CSS.

It's probably reasonable to leave the #content-profile-view element, but it would be advisable to switch it from a p to a div to make theming easier (using a p unnecessarily inherits all the p declarations used in other parts of a design). And I don't remember offhand what gets put in there, so I'm not sure this is the case, but if any block elements are inside a p that is invalid html. So better all around just to make it a div.

xjm’s picture

Status: Needs work » Needs review
FileSize
931 bytes

I'd agree with #7. This reroll of the patch replaces the <p> tag with with a <div> rather than removing it completely.

fago’s picture

Status: Needs review » Needs work

I agree with #7 too, but still just removing the
tags does just remove the spacing as I noted in #5. Let's re-add those space with css.

xjm’s picture

This CSS should do the trick:

div.content-profile-display {
  margin-bottom: 2em;
}

Edit: found the stylesheet. I'll roll a combined patch shortly.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.42 KB

Here. Had to bump the bottom margin to 3em to get the same amt. of whitespace by default.

fago’s picture

Status: Needs review » Needs work

ah, fine. However I noted that doesn't work for the links (add or view) yet - in that case still the whitespace is missing.

xjm’s picture

Hmm, there's no markup printed around the add/edit links, nor even a class on the link we could use. So, that'd need to be added to the module's rendering and then styled with CSS. Maybe it would be better to replace the <br /> with a div around content profile's area for each type and apply the bottom margin to that.

mpotter’s picture

Patch works well for me. I completely agree that modules should not use
for spacing (that is what CSS is for), and replacing the

with

helps fix HTML validation errors in the output (which ended up with

within

which isn't strictly allowed).

So +1 for patch.

atolborg’s picture

+1 for this change.

Pls’s picture

Yes, I agree that using br tags is totally wrong. Looks like we need a re-roll on #11 with some CSS for links margin's (probably would be good to wrap all links to a div and apply margin for that div). Let's do this together. Any volunteers?