Edgewall Software
Modify

Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#11106 closed enhancement (fixed)

Inputs and Text areas should have the same number of columns on the Report Edit page

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: Jun Omae
Priority: normal Milestone: 1.0.2
Component: report system Version: 1.0-stable
Severity: normal Keywords:
Cc:
Release Notes:

Title, Description and Query inputs on the report edit page are the same width.

API Changes:

Description

Attachments (4)

ReportEdit.png (39.9 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 6 years ago.
ReportEdit2.png (40.8 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 6 years ago.
ReportEdit3.png (45.0 KB ) - added by anonymous 6 years ago.
ReportEdit4.png (52.6 KB ) - added by anonymous 6 years ago.

Download all attachments as: .zip

Change History (26)

Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: ReportEdit.png added

comment:1 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Simple patch in 114561bf results in the following:

Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: ReportEdit2.png added

comment:2 Changed 6 years ago by Christian Boos

Milestone: 1.0.2
Owner: set to Christian Boos
Status: newassigned

Looks much better indeed, thanks!

Changed 6 years ago by anonymous

Attachment: ReportEdit3.png added

Changed 6 years ago by anonymous

Attachment: ReportEdit4.png added

comment:3 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

This isn't working as well as I had hoped because apparently a certain number of cols for an input element is not the same width as the same number of cols for a textarea across all browsers.

The patch in 238ca84a makes the following changes:

  • Centers the #content div (like on the ticket page).
  • Sets the width of the #content div to be 645px. This value was chosen because it was the width of the textarea in Chromium 25 when the number of cols was set to 78.
  • Sets the width of the input and textarea elements to be 100%.
  • Removes the cols attribute from the input and textarea elements.

Now it looks quite a bit better I think!:

comment:4 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

If this change looks good, then it might make sense to do similarly (i.e. center and limit width) for the Milestone Edit page.

comment:5 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

There is a problem with my patch. It causes the content of the Available Reports page to also be centered and have a max width of 645px. This can mostly be fixed by changing the selector from #content.report to its child #edit_report, however when that is done the h1 is not centered, and there is no way to uniquely select this h1 without adding a class or id to it or an enclosing element.

I can think of several fairly equivalent ways to fix this issue.

  1. Add an edit class to the #content div so that we can select the div on the edit page without selecting the div on the view page.
  2. Add a centered class to the #content div, which effectively does the same thing as the edit class, but we also refactor the ticket page to use this class.
  3. Wrap the h1 and form#edit_report in a div.

(1) seems like the better solution to me.

comment:6 Changed 6 years ago by Christian Boos

(1) is fine with me.

The problem with a centered class is that if people don't like / can't use a centered style, they'll have to create CSS rules for .centered which will be anything but centered…

comment:7 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

In 3ef54a8d: Add an edit class to the enclosing div on the report edit page and modify the CSS rules for centering the div on the report edit page so that the report view page isn't also centered. Make the report edit div to be the same width (58 em) as the ticket edit div

comment:8 Changed 6 years ago by Jun Omae

Looks good! However it has three failures in functional tests. In XHTML, the cols attribute of textarea is not optional.

...
TwillAssertionError:
# Element textarea does not carry attribute cols
# URL: http://127.0.0.1:8410/report?action=new
# Line 77, column 3
...
-------
Ran 127 tests in 592.780s

FAILED (failures=3)
make: *** [functional-test] Error 1

comment:9 in reply to:  8 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Replying to jomae:

Looks good! However it has three failures in functional tests.

Thanks, I'll fix those issues and submit a post a new branch with a cleaner commit history.

comment:10 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Issue from comment:8 fixed and changes rebased against 1.0-stable in t11106.2. Functional tests in trac/ticket/tests/functional.py pass now after commenting out all the entries including and after RegressionTestTicket4630a, due to the issue described in #11176.

comment:11 Changed 5 years ago by Ryan J Ollos

I no longer feel that these are the right CSS changes to make. It looks like width: 100% for input and textarea isn't effective for making the elements the same width since we need to take into account all of the dimensions of the bounding box.

comment:12 Changed 5 years ago by Ryan J Ollos

Milestone: 1.0.2next-stable-1.0.x
Owner: changed from Christian Boos to Ryan J Ollos

comment:13 Changed 5 years ago by Ryan J Ollos

Milestone: next-stable-1.0.x1.0.3

comment:14 in reply to:  11 Changed 5 years ago by Jun Omae

I no longer feel that these are the right CSS changes to make. It looks like width: 100% for input and textarea isn't effective for making the elements the same width since we need to take into account all of the dimensions of the bounding box.

I think that using box-sizing: border-box would be simple to make the elements the same width. See log:jomae.git:t11106.3.

comment:15 Changed 5 years ago by Ryan J Ollos

The element width looks good. Tested:

  • Windows 7
    • IE 11
    • Chrome 32
    • Firefox 24 (Waterfox)
    • Opera 18
  • Ubuntu 13.04
    • Chromium 31
    • Firefox 26
    • Opera 12.16

I do think we should consider limiting the width though (similar to the ticket form),

  • trac/htdocs/css/report.css

    diff --git a/trac/htdocs/css/report.css b/trac/htdocs/css/report.css
    index 0e1471b..a75e906 100644
    a b h2.report-result {  
    8282#report-descr { margin: 0 2em; font-size: 90% }
    8383#report-notfound { margin: 2em; font-size: 110% }
    8484
     85#content.report.edit {
     86 margin-left: auto;
     87 margin-right: auto;
     88 max-width: 58em;
     89}
    8590#content.report input#title, #content.report textarea.trac-resizable {
    8691 -moz-box-sizing: border-box;
    8792 box-sizing: border-box;
  • trac/ticket/templates/report_edit.html

    diff --git a/trac/ticket/templates/report_edit.html b/trac/ticket/templates/report_edit.html
    index 3d85e45..87f2cf6 100644
    a b  
    1111  </head>
    1212
    1313  <body>
    14     <div id="content" class="report">
     14    <div id="content" class="report edit">
    1515
    1616      <h1>${_('New Report') if action == 'new' else report.title}</h1>
    1717      <form action="${href.report(report.id)}" method="post" id="edit_report">

comment:16 Changed 5 years ago by Jun Omae

I don't think the edit form of report needs the narrow width and center-aligned. The view and edit form of ticket have the narrow width and center-aligned, that is consistent. However, the view of report is left aligned. When come and go between the view and edit form, my eyes would be moved between left side and center of the screen.

comment:17 Changed 5 years ago by Ryan J Ollos

Owner: changed from Ryan J Ollos to Jun Omae

comment:18 Changed 5 years ago by Ryan J Ollos

Since we don't have agreement on the width and alignment and there have been no other inputs, we could:

  • Apply the changes to 1.0-stable that keep the form left aligned but don't change the width: log:rjollos.git:t11106.4
  • Apply your other changes on the trunk, which would allow time for more user feedback before a major release (i.e. don't apply log:rjollos.git:1b587eace on the trunk).

comment:19 Changed 5 years ago by Ryan J Ollos

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

I pushed the changes we agreed on, to 1.0-stable in [12796] and merged to trunk in [12797]. Push the other change so that the elements span the width of the page if you would like, but please do it on the trunk so we can get feedback.

comment:20 Changed 4 years ago by Ryan J Ollos

It looks like we can simplify the CSS in [12796] a little bit:

  • trac/htdocs/css/report.css

    diff --git a/trac/htdocs/css/report.css b/trac/htdocs/css/report.css
    index 0155f76..8ae4772 100644
    a b h2.report-result {  
    8282#report-descr { margin: 0 2em; font-size: 90% }
    8383#report-notfound { margin: 2em; font-size: 110% }
    8484
    85 #content.report input#title, #content.report textarea.trac-resizable {
     85#content.report input#title, #content.report .trac-resizable {
    8686 -moz-box-sizing: border-box;
    8787 box-sizing: border-box;
    88 }
    89 #content.report input#title, #content.report div.trac-resizable,
    90 #content.report textarea.trac-resizable {
    9188 width: 100%;
    9289}
    9390#content.report.edit form {

This results in box-sizing: border-box being applied to div.trac-resizable, but it doesn't change the layout at all as far as I can tell.

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

comment:21 in reply to:  20 Changed 4 years ago by Ryan J Ollos

Replying to rjollos:

It looks like we can simplify the CSS in [12796] a little bit:

This code was refactored in #11678.

comment:22 Changed 4 years ago by Ryan J Ollos

Component: generalreport system

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted.
to 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.