Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

#11403 closed enhancement (fixed)

Definition terms in list have absolute positioning

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: general Version:
Severity: normal Keywords:
Cc: Jun Omae Branch:
Release Notes:

Refactored CSS for horizontal dl on changeset and diff pages to remove absolute positioning.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

The dt elements on the changeset and diff view pages have absolute positioning, so if someone wanted to say, add a left margin #overview { margin-left: 2.5em }, the entire contents of the dl would not be shifted:

—>

The following change seems to make sense, since it removes the absolute left position of the dts, and instead sets the margin-left on the parent to preserve their positioning.

  • trac/htdocs/css/diff.css

    diff --git a/trac/htdocs/css/diff.css b/trac/htdocs/css/diff.css
    index bc081cc..22db26c 100644
    a b  
    22#prefs fieldset { margin: 1em .5em .5em; padding: .5em 1em 0 }
    33
    44/* Diff/change overview */
    5 #overview { line-height: 130%; margin-top: 1em; padding: .5em }
     5#overview { line-height: 130%; margin-top: 1em; padding: .5em .5em .5em 0 }
    66#overview dt.property {
    77 font-weight: bold;
    88 padding-right: .25em;
    99 position: absolute; /* relies on #content { position: relative } */
    10  left: 0;
    1110 text-align: right;
    1211 width: 7.75em;
    1312}

After that change, if we set a margin-left in the custom stylesheet (#overview { margin-left: 2.5em }), we get:

There was some recent adjustments to the left property in [11097] and [11136]. Prior to that, the most recent change was [943].

Attachments (4)

After.png (11.7 KB ) - added by Ryan J Ollos 7 years ago.
Before.png (11.8 KB ) - added by Ryan J Ollos 7 years ago.
MarginLeft2.5em.png (11.7 KB ) - added by Ryan J Ollos 7 years ago.
changeset-r12330-20131217T115944.png (21.5 KB ) - added by Jun Omae 7 years ago.

Download all attachments as: .zip

Change History (14)

by Ryan J Ollos, 7 years ago

Attachment: After.png added

by Ryan J Ollos, 7 years ago

Attachment: Before.png added

comment:1 by Ryan J Ollos, 7 years ago

Description: modified (diff)

by Ryan J Ollos, 7 years ago

Attachment: MarginLeft2.5em.png added

comment:2 by Ryan J Ollos, 7 years ago

A few more changes to remove the need for absolute positioning:

  • trac/htdocs/css/diff.css

    diff --git a/trac/htdocs/css/diff.css b/trac/htdocs/css/diff.css
    index bc081cc..761858f 100644
    a b  
    22#prefs fieldset { margin: 1em .5em .5em; padding: .5em 1em 0 }
    33
    44/* Diff/change overview */
    5 #overview { line-height: 130%; margin-top: 1em; padding: .5em }
     5#overview { line-height: 130%; margin-top: 1em; padding: .5em .5em .5em 0 }
    66#overview dt.property {
     7 float: left;
    78 font-weight: bold;
    8  padding-right: .25em;
    9  position: absolute; /* relies on #content { position: relative } */
    10  left: 0;
     9 padding-right: .75em;
    1110 text-align: right;
    1211 width: 7.75em;
    1312}
    14 #overview dd { margin-left: 8em }
    1513
    1614#overview .message { padding: 1em 0 1px }
    1715#overview dd.message p, #overview dd.message ul, #overview dd.message ol,

These changes are based on a suggestion by Eric Ladner on the Bloodhound mailing list, and a review of the CSS for the dl-horizontal class in Bootstrap 2.x.

Prior to this patch, if you wanted to adjust the width of the#overview dt, you'd also have to adjust the margin-left of the #overview dd.

by Jun Omae, 7 years ago

in reply to:  2 ; comment:3 by Jun Omae, 7 years ago

The float: left would break the layout in changeset. See changeset-r12330-20131217T115944.png and r12330. I don't think that is good idea to use float: left.

Last edited 7 years ago by Jun Omae (previous) (diff)

in reply to:  3 comment:4 by Jun Omae, 7 years ago

Cc: Jun Omae added

Seems to need #overview dd { margin-left: ... }. Try the following.

  • trac/htdocs/css/diff.css

    diff --git a/trac/htdocs/css/diff.css b/trac/htdocs/css/diff.css
    index bc081cc..6658c5b 100644
    a b  
    22#prefs fieldset { margin: 1em .5em .5em; padding: .5em 1em 0 }
    33
    44/* Diff/change overview */
    5 #overview { line-height: 130%; margin-top: 1em; padding: .5em }
     5#overview { line-height: 130%; margin-top: 1em; padding: .5em .5em .5em 0 }
    66#overview dt.property {
     7 float: left;
     8 clear: left;
    79 font-weight: bold;
    8  padding-right: .25em;
    9  position: absolute; /* relies on #content { position: relative } */
    10  left: 0;
    1110 text-align: right;
    1211 width: 7.75em;
    1312}
    14 #overview dd { margin-left: 8em }
     13#overview dd { margin-left: 8.5em }
    1514
    1615#overview .message { padding: 1em 0 1px }
    1716#overview dd.message p, #overview dd.message ul, #overview dd.message ol,

comment:5 by Ryan J Ollos, 7 years ago

Milestone: 1.0.2
Status: newassigned

That makes sense I guess since the Bootstrap CSS also adds a margin-left on the dd. Thanks for spotting the need for clear: left as well.

I reviewed a few dozen additional changesets and it looks good for all those cases.

comment:6 by Ryan J Ollos, 7 years ago

After reviewing changeset.css, it seems there are some rules that aren't used anywhere (or any longer), and could therefore be removed on the trunk. Maybe I'm overlooking where they are used though. Proposed changes in log:rjollos.git:t11403.

comment:7 by Ryan J Ollos, 7 years ago

I've attempted to trace the history of the CSS that is being removed, to try and reduce the chance of making an error here.

[ecbae66a/rjollos.git]: There is no descendant dl from either of the elements with a #title id: branches/1.0-stable/trac/versioncontrol/templates/diff_form.html?rev=11308, branches/1.0-stable/trac/versioncontrol/templates/changeset.html?rev=11308. The CSS was added in [1767#file4] and applicable to a div#title in the diff.cs, which later became changeset.cs [2774]. It looks like the dl markup was removed in [1770].

$ grep -R "id=\"title\"" --exclude-dir=.svn
trac/ticket/templates/report_edit.html:            <input type="text" id="title" name="title" class="trac-focus" value="$report.title" size="50" /><br />
trac/versioncontrol/templates/diff_form.html:      <div id="title">
trac/versioncontrol/templates/changeset.html:      <div id="title" py:choose="" py:with="

I'll add a comment on [e85cf960/rjollos.git] later on.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:8 by Ryan J Ollos, 7 years ago

[e85cf960/rjollos.git]: CSS was added in [1109]. It looks like there was a nested div in the node_change function at that time, <div class="<?cs var:cl ?>"><div class="mod"></div></div>, but it's no longer present, <div class="$cl"> </div>: branches/1.0-stable/trac/versioncontrol/templates/changeset.html@11308:99#L93.

There are no div descendants from the #overview tag in diff_view.html, which is the other template with a #overview id.

$ grep -R id=\"overview\" --exclude-dir=.svn
trac/templates/diff_view.html:      <dl id="overview" py:with="multi = num_changes &gt; 1">
trac/versioncontrol/templates/changeset.html:      <dl id="overview">

comment:9 by Jun Omae, 7 years ago

I've investigated [e85cf960/rjollos.git] and [ecbae66a/rjollos.git], too. The two changes look good to me.

comment:10 by Ryan J Ollos, 7 years ago

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

Refactoring committed to 1.0-stable in [12349] and merged to trunk in [12350]. CSS removal committed to trunk in [12351].

Log message for [12349] is incorrect and should say Also removed an unused CSS rule from changeset.css. Log message for [12350] is incorrect, and should reference [12349] rather than [12340].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos 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.