Don't issue empty <dd> comment block in timeline

[Dirk Stöcker]

When the timeline contains entries without comment, then an empty <dd> block is created. Tidy complains about these.

Please simply don't create empty <dd>. Saves spaces and reduces the warnings.

[Jun Omae]

Reproduced it when attaching a file without comment.

Please try the following patch:

trac/timeline/templates/timeline.html

@@ -47 +47 @@
         <dl py:for="unread, events in groupby(events, key=lambda e: lastvisit and lastvisit &lt; e.dateuid)"
             class="${'unread' if unread else None}">
           <py:for each="event in events"
-            py:with="highlight = precision and precisedate and timedelta(0) &lt;= (event.date - precisedate) &lt; precision">
+                  py:with="highlight = (precision and precisedate and
+                                        timedelta(0) &lt;= (event.date - precisedate) &lt; precision);
+                           rendered = event.render('description', context)">
             <dt class="${classes(event.kind, highlight=highlight, unread=unread)}">
               <a href="${event.render('url', context)}" py:choose="">
                 <py:when test="event.author"><i18n:msg params="time, title, author">
@@ -59 +61 @@
                 </py:otherwise>
               </a>
             </dt>
-            <dd class="${classes(event.kind, highlight=highlight)}">
-              ${event.render('description', context)}
-            </dd>
+            <dd py:if="rendered" class="${classes(event.kind, highlight=highlight)}">${rendered}</dd>
           </py:for>
         </dl>
       </py:for>

[Dirk Stöcker]

Yes. Fixes the issue. A side effect is that also the separating space is removed.

A fix for this style issue:

trac/htdocs/css/timeline.css

@@ -45 +45 @@
 .timeline dt .time { color: #999; font-size: 80%; }
 .timeline dt .trac-author { color: #666; }
 .timeline dt.highlight { background-color: #ffa; }
+.timeline dt.nocomment { margin-bottom: .75em; }
 .timeline dd {
  font-size: 80%;
  margin: 0 0 .75em 5.8em;

And the <dt> class in above snippet must look like:

<dt class="${classes(event.kind, highlight=highlight, unread=unread, nocomment=False if rendered else True)}">

[Jun Omae]

I'd like to use dt + dt selector rather than .nocomment selector.

trac/htdocs/css/timeline.css

@@ -23 +23 @@
 }
 
 .timeline dt { background: 3px 4px no-repeat; padding: 0 }
+.timeline dt + dt { margin-top: 0.75em }
 .timeline dt :link, .timeline dt :visited {
  background: 3px 3px no-repeat;
  border: none;

[Dirk Stöcker]

Feel free to do so. I'm no CSS guru.

[Dirk Stöcker]

Any reason why this isn't simply applied instead of moving it from milestone to milestone?

[Ryan J Ollos]

Needs a test (TracDev/SubmittingPatches#Whatisagoodpatch).

[Dirk Stöcker]

So you leave a bug open for 11 months because you feel a unit test is required to check if a buggy feature really is properly fixed?

I could agree that unit tests are mandatory for new features, but not for bug fixes! So if you have a security hole and get a fix will you also reject it for a year due to missing unit test?

[Ryan J Ollos]

Committed to 1.2-stable in r16942, merged to trunk in r16943.