This is an inappropriate use of the label element in html.

#markup' => '<label>' . t('Did you mean') . '</label>' . l($new_keywords, $query->get_path($new_keywords)),

Should probably be:

#markup' => '<div class="label">' . t('Did you mean') . l($new_keywords, $query->get_path($new_keywords))  . '</div>',

For the moment it would also need to be styled with:

div.label {
  float: left; /* LTR */
  font-weight: normal;
  width: 10em;
}

if you want to have it look like a label. However, really this should just be emphasized text, so this would work well too:

#markup' => '<p><strong>' . t('Did you mean') . l($new_keywords, $query->get_path($new_keywords))  . '</strong></p>',

Comments

pwolanin’s picture

So label should only be used inside a form?

mgifford’s picture

It needs to be tied to a form element as per - http://www.w3schools.com/tags/tag_label.asp

Labels are always tied to a specific form element and it isn't good to use them for generic markup.

pwolanin’s picture

Status: Active » Needs review
StatusFileSize
new733 bytes

how about a dl?

pwolanin’s picture

StatusFileSize
new1.74 KB

or better yet, a theme function?

mgifford’s picture

I like the idea behind #4, but after applying it I didn't get the 'Did you mean' HTML being spit out to the screen.

I haven't had a chance to trace this down, but the themable function didn't show up.

jpmckinney’s picture

You probably need to clear your theme cache. Drupal won't pick up the new theme functions otherwise.

mgifford’s picture

I'd thought I'd done that, but it's working now, so perhaps not. Thanks.

This is a good patch that adds more flexibility in how this suggestion is displayed. Always useful

jpmckinney’s picture

Patch looks good. Not sure how dl/dts look with default styling in D7, though.

pwolanin’s picture

Here are a bunch of screen shots plus patches with EM and STRONG options showing the old styling and various new styling. By default a DL element seems to have a bit of excess margin on the top, but a quick hack to make it look better in Bartik is to add the "form-item" class (not really a lie since it's inside the FORM element).

Existing appearance w/ LABEL: http://drupal.org/files/issues/search-label.png

Appearance per #4 using STRONG: http://drupal.org/files/issues/search-dl-strong.png

Appearance per #4 using EM: http://drupal.org/files/issues/search-dl-em.png

Per #9 with added class using STRONG: http://drupal.org/files/issues/search-dl-form-item-strong.png

Per #9 with added class using EM: http://drupal.org/files/issues/search-dl-form-item-em-results.png

jpmckinney’s picture

I vote #9 with strong.

pwolanin’s picture

Version: 7.x-1.x-dev » 6.x-2.x-dev
Status: Needs review » Patch (to be ported)

ok, let's go with that for now - it's less visual change from what we have with LABEL.

committed to 7.x - do we want to port to 6.x-2.x or 6.x-1.x?

jpmckinney’s picture

Version: 6.x-2.x-dev » 6.x-1.x-dev

Will probably get more attention in the 1.x queue.

mgifford’s picture

Thanks! And agreed that most folks using this will presently be with the D6 issue queue.

pwolanin’s picture

Is this safe to backport?

mgifford’s picture

There is no reason not to back-port it that I can see. It's not proper HTML, but a legacy approach to formatting that is still in D7 in some places unfortunately.

pwolanin’s picture

still need the 6.x patch

pwolanin’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new71.59 KB
new78.89 KB
new1.77 KB

Here's a D6 patch plus screenshots to compare before and after the patch. The text looks basically the same in Garland, but the alignment changes a bit.

jpmckinney’s picture

I can live with the alignment change, but maybe others can't stand it.

nick_vh’s picture

Status: Needs review » Fixed

Looking good. Committed, one issue down - many more to go for the 6 version :-)

nick_vh’s picture

Status: Fixed » Closed (fixed)