Edgewall Software

Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#11499 closed enhancement (fixed)

Eliminate redundant code in milestone and admin milestone templates / modules — at Version 20

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: ticket system Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
  • Changed styling of Milestone, Ticket System Admin and Repository Admin edit pages by removing fieldsets around the Descriptions.
  • When milestone edit validations fail on the Milestone Admin page, a warning is added rather than redirecting to the error page.
API Changes:

Added methods to MilestoneModule class:

  • get_default_due
  • save_milestone

Add method to Milestone class:

  • get_num_tickets

New template milestone_edit_form.html for editing a milestone is used on the milestone_edit.html and admin_milestone.html pages. The MilestoneModule is used in trac.ticket.admin.MilestoneAdminPanel to eliminate redundant code.

Internal Changes:

Description

An important step towards #3754 (see comment:7:ticket:3754) is to merge portions of the milestone_edit.html and admin_milestones.html pages. It may be possible to eliminate some redundant code in trac.ticket.admin and trac.ticket.roadmap as well.

Change History (33)

comment:1 by Ryan J Ollos, 10 years ago

Owner: set to Ryan J Ollos
Status: newassigned

Changes to templates can be found in log:rjollos.git:t11499. I'm still working on changes to the request handling, which are a bit more complex, and then more tests need to be added.

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

comment:2 by Ryan J Ollos, 10 years ago

I committed the changes in comment:1 in [12858:12860]. I'll proceed by looking at what code we can reuse from MilestoneModule in MilestoneAdminPanel.

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

comment:3 by Ryan J Ollos, 10 years ago

Milestone: 1.1.31.1.2

There are a few minor defects in [12858]. I'll fix those and add more functional tests.

comment:4 by Ryan J Ollos, 10 years ago

The issues with [12858] are:

  • The textarea is not readonly when user doesn't have MILESTONE_MODIFY.
  • The submit button is not disabled when user doesn't have MILESTONE_MODIFY.

One other small detail: I can't see a need for both the hidden form field id trunk/trac/ticket/templates/milestone_edit.html@12857:58#L49 and parsing the id from the path: trunk/trac/ticket/roadmap.py@12857:658#L652. The field in the template has been there since [600#file1]. The parsing of id has existed since match_request was implemented in [1586#file14]. It seems like one of the two could be removed.

comment:5 by Ryan J Ollos, 10 years ago

Issues described in comment:4 fixed in [12870]. Improvements to template and styling in [12869,12871:12872].

in reply to:  description comment:6 by Ryan J Ollos, 10 years ago

Replying to rjollos:

It may be possible to eliminate some redundant code in trac.ticket.admin and trac.ticket.roadmap as well.

Extensive changes to trac.ticket.admin and trac.ticket.roadmap in log:rjollos.git:t11499.3. The changes eliminate redundant code and should ease the implementation of #3754.

On the admin milestone page, TracErrors raised on POST have been replaced with a warning and a redirect back to the edit page.

comment:7 by Jun Omae, 10 years ago

We should provide i18n/l10n message in the legend element of [12781].

-      <legend py:if="compact" py:content="'Modify Milestone:' if can_modify else 'View Milestone:'"></legend>
+      <legend py:if="compact" py:choose="">
+        <py:when test="can_modify" i18n:msg="">Modify Milestone:</py:when>
+        <py:otherwise i18n:msg="">View Milestone:</py:otherwise>
+      </legend>

BTW, <fieldset py:strip="not compact">...<legend py:if="compact">, it doesn't seem to be elegant…. I think we could use <py:def function="...">.

comment:8 by Ryan J Ollos, 10 years ago

One additional minor styling change in [12875].

I'll look into the suggestions from comment:7 shortly.

in reply to:  7 ; comment:9 by Ryan J Ollos, 10 years ago

Replying to jomae:

BTW, <fieldset py:strip="not compact">...<legend py:if="compact">, it doesn't seem to be elegant…. I think we could use <py:def function="...">.

Yeah, it's not very elegant or maintainable. Initially I was trying to keep the templates unchanged for a pure refactoring. However, it might make sense to change the layout and styling a little bit. I've eliminated the compact variable now:

I have a few more tweaks to make and then I'll post changes for review. The screen captures above are just to illustrate the basic idea.

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

by Ryan J Ollos, 10 years ago

Attachment: AdminMilestoneEdit.png added

by Ryan J Ollos, 10 years ago

Attachment: RoadmapMilestoneEdit.png added

by Ryan J Ollos, 10 years ago

Attachment: CreateReport.png added

by Ryan J Ollos, 10 years ago

Attachment: EditReport.png added

by Ryan J Ollos, 10 years ago

Attachment: RetargetingNoTickets.png added

by Ryan J Ollos, 10 years ago

Attachment: RetargetingFieldset.png added

comment:10 by Ryan J Ollos, 10 years ago

Some additional changes:

  • Of the existing forms in Trac for creating resources, the Report Edit form is the only one that does not contain a fieldset. The following changes are proposed proposed for 1.0-stable. The legend content is Create Report: when the report does not exist, and Modify Report: when the report exists. Note that the submit buttons are located below the fieldset.

  • The Submit buttons on the Wiki Edit and New Ticket pages are located below the fieldset. This change was made to the Milestone Edit form and the edit forms for the other ticket system objects.
  • Added a fielset around the retargeting elements:

in reply to:  9 ; comment:11 by Jun Omae, 10 years ago

Replying to rjollos:

Replying to jomae:

BTW, <fieldset py:strip="not compact">...<legend py:if="compact">, it doesn't seem to be elegant…. I think we could use <py:def function="...">.

Yeah, it's not very elegant or maintainable. Initially I was trying to keep the templates unchanged for a pure refactoring.

PoC for use of <py:def function=...> is like this. It's a bit maintainable, but not elegant.

  <py:def function="_top">
    <div class="field">
      <input type="hidden" name="id" value="$milestone.name" />
      <input type="hidden" name="action" value="edit" />
      <label>Name:<br />
        <input type="text" id="name" name="name" class="trac-autofocus" size="32"
               value="${milestone.name or req.args.get('name')}" readonly="$readonly" />
      </label>
    </div>
  </py:def>
  <py:def function="_middle">...</py:def>
  <py:def function="_bottom">...</py:def>
  <form id="edit" class="mod" method="post" action=""
        py:with="disabled = 'disabled' if can_modify is False else None;
                 readonly = 'readonly' if can_modify is False else None"
        py:choose="">
    <fieldset py:when="compact">
      <legend py:choose="">
        <py:when test="can_modify" i18n:msg="">Modify Milestone:</py:when>
        <py:otherwise i18n:msg="">View Milestone:</py:otherwise>
      </legend>
      ${_top()}
      ${_middle()}
      ${_bottom()}
    </fieldset>
    <py:otherwise>
      ${_top()}
      <fieldset>
        <legend>Schedule</legend>
        ${_middle()}
      </fieldset>
      ${_bottom()}
    </py:otherwise>
  </form>
</html>

However, it might make sense to change the layout and styling a little bit. I've eliminated the compact variable now: […]

The changes are reasonable and these screenshots looks good to me. It might be good without the Schedule fieldset and legend elements because nested fieldset elements are verbose.

Replying to rjollos:

  • The Submit buttons on the Wiki Edit and New Ticket pages are located below the fieldset. This change was made to the Milestone Edit form and the edit forms for the other ticket system objects.

Agreed. I think that's good styling to locate the buttons outside the fieldset.

comment:12 by Ryan J Ollos, 10 years ago

Thanks. I'll experiment without the Schedule fieldset. When you say the nested fieldsets are verbose, do you mean the page looks cluttered, or are you referring to accessibility (e.g. how a screen reader would act on the content)?

I created #11664 for the changes to the Report Edit page.

in reply to:  11 comment:13 by Ryan J Ollos, 10 years ago

Replying to jomae:

Replying to rjollos:

  • The Submit buttons on the Wiki Edit and New Ticket pages are located below the fieldset. This change was made to the Milestone Edit form and the edit forms for the other ticket system objects.

Agreed. I think that's good styling to locate the buttons outside the fieldset.

I created #11666 to apply the change to 1.0-stable. Although on second thought it may be better to apply the change to the trunk since it could cause problems for a plugin XPath selector in an ITemplateStreamFilter.

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

in reply to:  7 comment:14 by Ryan J Ollos, 10 years ago

Replying to jomae:

We should provide i18n/l10n message in the legend element of [12781].

Fixed in [12932].

Additional changes in

Message extractions in [12933,12937,12939].

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

in reply to:  4 comment:15 by Ryan J Ollos, 10 years ago

Replying to rjollos:

One other small detail: I can't see a need for both the hidden form field id trunk/trac/ticket/templates/milestone_edit.html@12857:58#L49 and parsing the id from the path: trunk/trac/ticket/roadmap.py@12857:658#L652. The field in the template has been there since [600#file1]. The parsing of id has existed since match_request was implemented in [1586#file14]. It seems like one of the two could be removed.

The hidden field was removed in [12940].

comment:16 by Ryan J Ollos, 10 years ago

Some proposed changes in log:rjollos.git:t11499.5:

  • Removes the Schedule fieldset.
  • Removes the fieldset around the Description on several pages.
  • Adds a fieldset around the Retarget elements.

I still need to tweak the CSS a bit. The margins around the Description textarea and label are not good now.

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

by Ryan J Ollos, 10 years ago

by Ryan J Ollos, 10 years ago

by Ryan J Ollos, 10 years ago

by Ryan J Ollos, 10 years ago

by Ryan J Ollos, 10 years ago

Attachment: EditMilestone_20140714.png added

by Ryan J Ollos, 10 years ago

comment:17 by Ryan J Ollos, 10 years ago

Screen captures showing the latest:

Admin Milestone Edit page (with and w/o MILESTONE_MODIFY)

Admin Version and Component Edit pages

Milestone Edit page

comment:18 by Ryan J Ollos, 10 years ago

The changes in log:rjollos.git:t11499.6 implement what is shown in the screen capture above.

comment:19 by Ryan J Ollos, 10 years ago

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

Changes committed to trunk in [12964:12967].

comment:20 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Note: See TracTickets for help on using tickets.