Feedback doesn't play nicely with Zen Classic Theme

I'm not working with Zen themes, so someone else needs to provide a patch for this issue.

hmm, same here – Zen.

It floats to the right for me too... but this in itself isn't bad for my site layout -

But the real problem, like above – When I try to click in the popup, it just closes again (maybe it's the same, no edit form so clicking registers as trying to close it).

Look forward to any suggestions... maybe this issue could move to the zen theme issue queue?

Cheers!
:) Scott

Attached is the patch that makes the feedback "box" work like expected on (hopefully) all themes.

Explanation

To wrap the title of feedback block into a clickable div (for the toggle) effect, the following code was used -

  $block = $('#block-feedback-form');
  $block.children().eq(0)
    .wrap('<div class="feedback-link"></div>').parent()

which is wrong because the title need not always be the first child of div#block-feedback-from (in zen the first child is div#block-inner)

So the patch changes the above code to

  $block = $('#block-feedback-form');
  $block.find("h2").eq(0)
    .wrap('<div class="feedback-link"></div>').parent()

(It is assumed that even custom block.tpl.php files of all themes will have the block title inside the first h2 tags in the block)

The only problem I have with this patch is that "h2" is a similar assumption, too. Block templates can validly use h3, h4 or even h5. With jQuery 1.2, we might use ":heading", but would have to make sure that we are really selecting the block's title and no other title.

Ofcourse a developer might use any HTML tag for block titles in templates. In such a case i think the only way to have a completely generic solution is for us to wrap the feedback-block's title into something unique and modify the js accordingly.

Let me know if the following is acceptable so that i can write a full-proof patch for this issue
$block['subject'] = t('<span id="feedback-block-title">Feedback</span>');

Meanwhile I have encountered another bug and filed an issue here

Well, that might work. However, we should then remove .wrap('<div class="feedback-link"></div>') and unnecessary code from the JS and use

$block['subject'] = '<div class="feedback-link">'. t('Feedback') .'</div>';

instead then.

What was i thinking :P

Your suggestion is much better. I have attached a new patch that uses the above idea. It has necessary changes for the js. Might look like the patch has an extra/redundant span, but that was necessary to maintain the 12px font consistency.

I have tested the patch to my satisfaction by using all kinds of block templates. Works in all the cases (as long as $block->subject is used in the template of course)

Looks better. However, two caveats:

- This is unnecessary duplicate code, should use $block as context for selecting div.feedback-link:

$block = $('#block-feedback-form');
$('#block-feedback-form div.feedback-link')...

- Can we move #feedback-form-toggle back into the JS? I know it's not perfect either way, but my hope is to have Feedback degrading gracefully for users without JS some time in the future (which means that the form won't be collapsible)

Fair enough. Guess you were trying to guide me to the present patch.

Great! I did not test the patch yet, but if it works, I only have minor coding-style issues:

- This line should be kept as is (note the trailing space and indentation):

-    .prepend('<span id="feedback-form-toggle">[ + ]</span> ')
+  	.prepend('<span id="feedback-form-toggle">[ + ]</span>')

- If we pass $block instead of node to feedbackFormToggle(), the first argument should be $block and we probably do not need $($block) in that function then :)

Please set the status to PNR afterwards.

It works like a charms for my custom theme. Thanks a lot!

PS: tested with Chamaleon, and it works gr8.

This last patch doesn't validate with a XHTML 1.0 Strict layout, because DIV is not permitted inside a H2 (or H3, H4)
I'm not capable to do a patch file, but i resolve this problem changing

$block.find('div.feedback-link')

with

$block.find('span.feedback-link')

in javascript file and changing
$block['subject'] = '<div class="feedback-link"><span>'.t('Feedback').'</span></div>';
with
$block['subject'] = '<span class="feedback-link"><span>'.t('Feedback').'</span></span>';

in .module file.

Tested against Firefox 3 and Opera 9.5 (Linux) and W3C HTML validator

Updated patch according to #10 and #12. Please test.

This works!

Another issue I had was that the block doesn't float.
adding div before #block-feedback-form { in the css fyle fixed that.

@asak: Which patch in this issue did you test? And, could you please elaborate a bit more on the CSS fix you needed to apply?

I tested the patch you submitted in #13, and had additionaly to change in the css:

instead of:
#block-feedback-form {
Use:
div#block-feedback-form {

And everything works... thanks!

Uhm. #block-feedback-form appears a lot of times in feedback.css - which one do you mean or do you mean all?

I get this too, but in D6. Hopefully someone will port the patch.

@sun:

This is what the top of the .css looks like now:

/* $Id: feedback.css,v 1.1.2.6 2008/07/16 11:14:48 sun Exp $ */

/* Reset commonly set styles */
#block-feedback-form,
#block-feedback-form .feedback-link,
#block-feedback-form .feedback-link *,
#block-feedback-form .content,
#block-feedback-form form,
#block-feedback-form form label {
  float: none;
  margin: 0;
  padding: 0;
  border: 0;
  font-size: 100%;
  font-weight: normal;
  color: inherit;
}
div#block-feedback-form {
  position: fixed;
  bottom: 2px;
  right: 7px;
  overflow: hidden;
  z-index: 10;
}

And it works... ;)

Thanks, committed!

has this been committed to the 6.x version (or was it not a problem there)?
:) Scott

It has been committed to all branches.

great - thx for clarifying sun :)

worth noting at date of this writing, it is committed to all branches in the development snapshots (e.g. 5.x-2.x-dev - silly me: I had read "all branches" to mean the stable releases; another lesson that cost nothing but time and a little pride :-* ).