We need to review the comment.tpl and make sure the HTML structure, the CSS, the variable names, and the contents of the variables are the best they can be for D7.

I apologize for not having a concrete list of to-dos to start out the discussion. :-(

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kika’s picture

core/stark comment.tpl.php:

<div class="<?php print $classes; ?> clearfix">

garland:

<div class="<?php print $classes . ' ' . $zebra; ?>">
  <div class="clearfix">  

- $zebra should documented in the core comment.tpl.php
- $zebra should added to outer div in core comment.tpl.php
- any specific reason why Garland uses extra wrapper and not clearfixing in outer div?

core:

<span class="new"><?php print $new ?></span>

garland:

<span class="new"><?php print drupal_ucfirst($new) ?></span>

- why we ucfirst only in garland, not in core? where else we ucfirst?

core:

<?php if ($signature): ?>
<div class="user-signature clearfix">
  <?php print $signature ?>
</div>

garland:

<?php if ($signature): ?>
<div class="clearfix">
  <div>—</div>
  <?php print $signature ?>
</div>
<?php endif; ?>

- <div>—</div> seems really sloppy
- why not have "user-signature" class for Garland?

core:

<?php print $links ?>

garland:

<?php if ($links): ?>
  <div class="links"><?php print $links ?></div>
<?php endif; ?>

- Why not have a links div class in core?

Looking at all this -- why we need garland's comment.tpl.php at all? Why not just clean up the core one, bringing over the sensible bits from garland's comment template?

kika’s picture

Status: Active » Needs work
FileSize
1.14 KB

Here's the patch to introduce the suggestions for Stark's comment.tpl.php

- Document $zebra variable
- Adding $zebra variable to outer div
- Wrapping $links into a div class="links" class

Next steps:

1. Test the patch with user pictures (either my installation or current state of CVS build are acting up)

2. Drop temporarily Garland comment.tpl.php and see what visually breaks in Garland comments. Likely a lot, because:

- Garland uses extra wrapper div for clearfix

- different order of variables:
-- core: $picture, $new, $title, $submitted
-- Garland: $submitted, $new, $picture, $title

- span instead of div for $submitted

3. Fix those visual issues based on proposed core comment.tpl.php and Garland CSS

4. Remove Garland's comment.tpl.php

Any takers?

kika’s picture

Next steps bit further in the horizon: as comments will become fieldable and their output will be converter to render arrays #504666: Make comments fieldable we might need to move $links being part of the $content[] array (in style of #339929: Move node links into $node->content http://drupal.org/node/224333#node_links) and re-engineer hook_link() and hook_link_alter() but this is way beyond of this issue.

kika’s picture

Back with screenshots: Garland does break when comments are based on stark comment.tpl.php

The problems:

garland/style.css 233:

span.submitted, .description, .vertical-tab-button .summary {
  font-size: 0.92em;
  color: #898989;
}

replacing span.submitted with div.submitted or .submitted gets us back the styling. Which one is preferred?

I do not have enough skills to fix the submitted float. As said in #2, Garland uses a different order of $title and $submitted.

kika’s picture

Process for Garland testing:

1. Apply the patch from #2
2. Rename themes/garland/comment.tpl.php to something else
3. Reset theme cache (easiest way is to visit /admin/build/themes/ and submit)

geerlingguy’s picture

Marking for subscribe. If I have some time later this evening, I'll hop in and try to do more of the steps outline in #2.

catch’s picture

Bump. This can probably go in during code slush, but needs to happen pretty quick.