this is one of the patches which was part of refactor profile_browse() and cleanup+new small features of profile.module and user.module

there were two ways to fix this - but nothing had to do with the function mentioned

  • at profile_view_profile() there is the following line: 
    $title = ($field->type != 'checkbox') ? check_plain($field->title) : '';

    one solution would be to change the '' to NULL.
    the ":" which is listed on the user-page does disappear (a common example is http://scratch.drupal.org/user/1 - do you see all the single ":" on every item?)
    but then text will be attached to the last item visually (because of the definition-list used)

  • change "the root of the evil":
    --> change theme_user_profile() - that is what I did
CommentFileSizeAuthor
#8 user.module_43.patch2.12 KBZen
#5 user.module_42.patch1.25 KBZen
#3 user.module_41.patch2.1 KBZen
#1 user.module_40.patch826 bytesZen
usermodule.fixthecolon.patch895 bytesTobias Maier

Comments

Zen’s picture

Zen commented
StatusFileSize
new user.module_40.patch826 bytes

Tobias, I didn't see your patch before.. but I've attached a patch which might do this a little more cleanly.

Ideally, we should change 'checkbox' to 'checkboxes' in profile.module, and use a list to display them - this is kinda ugly, and too labourious (adding in checkboxes one by one).

Please review - thanks :)
-K

Tobias Maier’s picture

Tobias Maier commented

your right, your patch is better :D

Zen’s picture

Zen commented
StatusFileSize
new user.module_41.patch2.1 KB

On second though, I just realised that the !empty will invalidate any title of value '0'. While the current profile module doesn't follow the form->validate->submit model, and invalidates titles with value 0.. they will be valid once the module has been converted.

A title value of 0 for a checkbox is a not so unlikely possibility.

Patch:

  • sets checkbox titles to NULL as per Tobias' original observation.
  • does not display the dt element if title==NULL. This validates fine and I couldn't find anything wrong with using a definition list with a dd but without the dt element. Empty elements == bad.
  • removes the \ns, as we don't use it with any degree of consistency to let them remain.
  • moves the ':' into a t().

As mentioned in my previous post, using a checkboxes item in the profile module (with a title field for the group) should sort this out eventually.

Cheers
-K

Tobias Maier’s picture

Tobias Maier commented

hmm...

your right an empty <dt> is bad.
but the structured information which we get is broken if we don't print a <dt>. (and it looks ugly - because it gets attached to the last <dt>)
because we generate a <dd> without a <dt>. That means that this <dd>-Element belongs to the last <dt>-Element.
Maybe we should print an element without a valid title in a <div>

I prefer the following check

if (!empty($item['title']) || $item['title'] == 0) {
...
}

I think we should combine both setting it to NULL and to check if it is empty

t(':')
Why? I dont see any need for this.
Just one sql-query more...

Zen’s picture

Zen commented
StatusFileSize
new user.module_42.patch1.25 KB

The dd tag will be inside the dl, so there shouldn't be any confusion with the previous dt.

The t() doesn't involve a separate sql query. I've refined it further in the attached patch as per Berkes' suggestion.

Please re-review :)

Thanks
-K

Zen’s picture

Zen commented
Title: senseless colon on user-page » Profile module checkbox elements mangle user page output
Priority: Normal » Minor

Changing title to be more accurate.

Thanks
-K

Tobias Maier’s picture

Tobias Maier commented

did you forget the part of the profile.module in this patch?

Zen’s picture

Zen commented
StatusFileSize
new user.module_43.patch2.12 KB

I did. Thanks :)

-K

Tobias Maier’s picture

Tobias Maier commented 
t('%title:', array('%title' => $item['title']))

did we ever something like this this way?
I searched a little bit in drupal core and I found this theme_form_element()

there you can read:

$output .= ' <label>'. $title .":$required</label>\n";

as you can see the ":" is not in a t()

I think this is not really needed -> so remove this part please
because I think then it is ready to go...

Zen’s picture

Zen commented

Thanks for the review Tobias :) But I disagree. This should be translatable. Not all languages might use a colon for punctuation etc. etc. For e.g. in Sanskrit, a full stop/period is usually represented by a | or a || depending on the context. The same might be the case with other punctuation symbols as well. Either way, I suggest that we RTC this and see what happens :D

If it happens elsewhere, they are bugs too :P [grep -Rin "\.':" * | wc -l == 16]

Thanks
-K

Tobias Maier’s picture

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

ok I understand....

dries’s picture

dries commented

Committed to HEAD. Thanks.

Zen’s picture

Zen commented
Status: Reviewed & tested by the community » Fixed
Anonymous’s picture

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