Edgewall Software
Modify

Opened 2 years ago

Closed 5 months ago

#12946 closed defect (fixed)

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

Reported by: Dirk Stöcker Owned by: Jun Omae
Priority: normal Milestone: 1.2.4
Component: timeline Version: 1.2.2
Severity: minor Keywords:
Cc: Branch:
Release Notes:

Fixed instance of empty comment block in timeline.

API Changes:

Description

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.

Attachments (0)

Change History (10)

comment:1 by Jun Omae, 2 years ago

Component: generaltimeline
Milestone: 1.2.3
Owner: set to Jun Omae
Status: newassigned

Reproduced it when attaching a file without comment.

Please try the following patch:

  • trac/timeline/templates/timeline.html

    diff --git a/trac/timeline/templates/timeline.html b/trac/timeline/templates/timeline.html
    index 4ebb4de68..b97ea9abb 100644
    a b  
    4747        <dl py:for="unread, events in groupby(events, key=lambda e: lastvisit and lastvisit &lt; e.dateuid)"
    4848            class="${'unread' if unread else None}">
    4949          <py:for each="event in events"
    50             py:with="highlight = precision and precisedate and timedelta(0) &lt;= (event.date - precisedate) &lt; precision">
     50                  py:with="highlight = (precision and precisedate and
     51                                        timedelta(0) &lt;= (event.date - precisedate) &lt; precision);
     52                           rendered = event.render('description', context)">
    5153            <dt class="${classes(event.kind, highlight=highlight, unread=unread)}">
    5254              <a href="${event.render('url', context)}" py:choose="">
    5355                <py:when test="event.author"><i18n:msg params="time, title, author">
     
    5961                </py:otherwise>
    6062              </a>
    6163            </dt>
    62             <dd class="${classes(event.kind, highlight=highlight)}">
    63               ${event.render('description', context)}
    64             </dd>
     64            <dd py:if="rendered" class="${classes(event.kind, highlight=highlight)}">${rendered}</dd>
    6565          </py:for>
    6666        </dl>
    6767      </py:for>

comment:2 by Dirk Stöcker, 2 years ago

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

    old new  
    4545.timeline dt .time { color: #999; font-size: 80%; }
    4646.timeline dt .trac-author { color: #666; }
    4747.timeline dt.highlight { background-color: #ffa; }
     48.timeline dt.nocomment { margin-bottom: .75em; }
    4849.timeline dd {
    4950 font-size: 80%;
    5051 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)}">

comment:3 by Jun Omae, 2 years ago

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

  • trac/htdocs/css/timeline.css

    diff --git a/trac/htdocs/css/timeline.css b/trac/htdocs/css/timeline.css
    index 8097df11c..f7c8279f5 100644
    a b  
    2323}
    2424
    2525.timeline dt { background: 3px 4px no-repeat; padding: 0 }
     26.timeline dt + dt { margin-top: 0.75em }
    2627.timeline dt :link, .timeline dt :visited {
    2728 background: 3px 3px no-repeat;
    2829 border: none;

comment:4 by Dirk Stöcker, 2 years ago

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

comment:5 by Ryan J Ollos, 17 months ago

Milestone: 1.2.31.2.4

comment:6 by Ryan J Ollos, 15 months ago

Milestone: 1.2.41.2.5

comment:7 by Dirk Stöcker, 15 months ago

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

comment:9 by Dirk Stöcker, 15 months ago

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?

comment:10 by Ryan J Ollos, 5 months ago

Milestone: 1.2.51.2.4
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.