Edgewall Software
Modify

Opened 2 years ago

Closed 4 months ago

Last modified 4 months ago

#10994 closed enhancement (fixed)

Add drag and drop and other dynamic behavior to the admin panels containing enum lists

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: rjollos
Priority: normal Milestone: 1.1.2
Component: admin/web Version:
Severity: normal Keywords:
Cc: leho@…
Release Notes:

Added Select All checkboxes to the admin Components, Milestones, Priorities, Repositories, Resolutions, Severities, Ticket Types and Versions tables.

API Changes:

The function addSelectAllCheckboxes adds a Select All checkbox to the heading of a table, when executed on jQuery object that wraps a table. The tds containing the checkboxes must have the sel class.

Description (last modified by rjollos)

A reworked version of th:AdminEnumListPlugin will be provided as a patch to Trac. The primary behavior will apply to the Priorities, Resolutions, Ticket Types and Severities panels, but behaviors will be implemented for almost every admin panel.

The following behaviors will be provided in the initial version of the patch:

  • The enum lists can be sorted by drag and drop of the items.#11682
    • The Order column of select elements is hidden.
    • A Revert changes button is added. The button is disabled until the list is reordered.
  • The Save changes button is disabled until a change is made.
  • The Remove selected items button is disabled until a checkbox is selected.#11056
  • Addition of a Select All checkbox in the table header.
  • Allow the Components, Milestones and Versions tables to be sorted (in jQuery?: th:#10876). For inspiration, see th:TableSorterPlugin. #11683

Similar behavior will be applied to other admin pages for consistency, where applicable.

Attachments (1)

t10994-r11508-1.patch (6.6 KB) - added by Ryan J Ollos <ryan.j.ollos@…> 23 months ago.
Patch against r11508 of the trunk.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 2 years ago by rblank

Sounds like a nice idea. Are you going to replace the current implementation completely, or are you going to build on the current (non-dynamic) functionality? That is, will the pages still work with JavaScript disabled?

We have always had a tradition in Trac to have static fallbacks for dynamic behavior. I'm not saying we should continue this forever (actually, I would rather advocate the opposite), just curious about your plan.

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

I'm not planning to change the templates or Python code at all, so it should work the same when JavaScript is disabled. For example, the select elements are left in place and hidden by the Javascript. I took the same approach in #9609.

The latest version of the th:AdminEnumListPlugin has all the functionality I have in mind. It is basically just a piece of javascript that should be reworked to make use of jQuery UI's Sortable widget, and all of the backward compatibility stuff can be dropped. It currently works in Trac 0.11, which required a significant number of workarounds.

comment:3 Changed 2 years ago by lkraav <leho@…>

  • Cc leho@… added

Changed 23 months ago by Ryan J Ollos <ryan.j.ollos@…>

Patch against r11508 of the trunk.

comment:4 follow-up: Changed 23 months ago by Ryan J Ollos <ryan.j.ollos@…>

  • Description modified (diff)

Updated description with my current thinking on what the final patch will contain.

I've implemented the portion of the patch that enables buttons when checkboxes or a file have been selected. I still have a bit of work to do on the other features, but wanted to post this version of the patch to ask a few questions that will help me complete the implementation. I'll highlight a few parts of the patch that I'm uncertain about and looking for early feedback on:

  1. I've put the disableIfNoneChecked function in trac.js with the idea that it could be useful outside of the administration panels. Specifically, I'm thinking we could use it for the Change tickets button on the batch modify form, and for the Add attachment button on the attachment page.
  2. I'm confused about where I should be placing the CDATA section tags. I don't see any reason to not just place them within every script text=text/javascript element, but I see them very selectively placed in some files.
  3. I've used this pattern change(function() {...}).eq(0).change() (in this case click(function() {...}).eq(0).click() would probably be the same), rather than .click(someFunction); someFunction(); (i.e. passing a function to click and then calling that function directly). Using change seems a bit cleaner and both patterns are present throughout the Trac codebase.

comment:5 in reply to: ↑ 4 ; follow-ups: Changed 23 months ago by cboos

Replying to Ryan J Ollos <ryan.j.ollos@…>:

  1. I've put the disableIfNoneChecked function in trac.js with the idea that it could be useful outside of the administration panels. Specifically, I'm thinking we could use it for the Change tickets button on the batch modify form, and for the Add attachment button on the attachment page.

Sounds good (for the attachment use case, I suppose there will be some additional check, for the file input).

  1. I'm confused about where I should be placing the CDATA section tags.

We apparently do it only when needed (i.e. when using plain '<', '&' or '>' characters in the Javascript code), but I wouldn't object using it everywhere for consistency (and for not having to think about it!).

  1. I've used this pattern change(function() {...}).eq(0).change() (in this case click(function() {...}).eq(0).click() would probably be the same), rather than .click(someFunction); someFunction(); (i.e. passing a function to click and then calling that function directly). Using change seems a bit cleaner and both patterns are present throughout the Trac codebase.

I suppose it all depends on whether there's a side-effect or not, and if yes, if the side-effect it predictable, or not (e.g. what would happen if there would be other event handlers?). The second pattern ensure you have a strict control over what you force during this initial call.

comment:6 in reply to: ↑ 5 Changed 23 months ago by Ryan J Ollos <ryan.j.ollos@…>

Replying to cboos:

Sounds good (for the attachment use case, I suppose there will be some additional check, for the file input).

I didn't describe it well, but I was thinking of putting a second function in trac.js that would be used for file / button pairs, such as Add attachment. Although, it might not be necessary since it seems possible to accomplish this in just 3 lines.

  1. I'm confused about where I should be placing the CDATA section tags.

We apparently do it only when needed (i.e. when using plain '<', '&' or '>' characters in the Javascript code), but I wouldn't object using it everywhere for consistency (and for not having to think about it!).

Not having to think about it sounds good to me. Also, the code is a bit cleaner when the tags aren't interleaved.

It looks like I was responsible for confusing myself on this issue. The only cases that I see the tags being present when they are not required is due to a changeset from one of my patches [t 11104].

comment:7 Changed 22 months ago by cboos

  • Milestone set to 1.0.2
  • Owner set to cboos
  • Status changed from new to assigned

comment:8 Changed 22 months ago by Ryan J Ollos <ryan.j.ollos@…>

Did you mean to target this to 1.0.2, or rather than 1.1.2? I'm happy to have that happen, but usually changes like those suggested in this ticket are targeted to major release.

I'll try to have a more extensive patch uploaded soon.

comment:9 follow-up: Changed 22 months ago by cboos

  • Milestone changed from 1.0.2 to next-dev-1.1.x

Ah, I was just thinking about the current patch t10994-r11508-1.patch which doesn't contain anything potentially disruptive for 1.0.x. I suppose that part could be added to 1.0.x and the more ambitious change which this ticket is really about would better be suited for 1.1.x indeed.

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

I like the idea of committing the existing patch separately. The existing patch is actually a bit of a deviation from the key things in mind that I had for this ticket anyway. I'll touch-up that patch and create a new ticket for it, before continuing with the other changes described for this ticket.

comment:11 in reply to: ↑ 9 Changed 22 months ago by Ryan J Ollos <ryan.j.ollos@…>

I've created #11056 for the part that has been discussed for 1.0.2, and will create a new version of the patch soon.

comment:12 in reply to: ↑ 5 Changed 22 months ago by Ryan J Ollos <ryan.j.ollos@…>

  • Description modified (diff)

Replying to cboos:

  1. I'm confused about where I should be placing the CDATA section tags.

We apparently do it only when needed (i.e. when using plain '<', '&' or '>' characters in the Javascript code), but I wouldn't object using it everywhere for consistency (and for not having to think about it!).

In the latest patch for #11056, I've included CDATA tags for almost all of the changed code in script elements. The one case I've found where this shouldn't be done is if there is embedded Genshi in the script, such as <py:if test="preview">...</py:if>. I guess that's obvious in retrospect since we need that Genshi to be parsed!

  1. I've used this pattern change(function() {...}).eq(0).change() (in this case click(function() {...}).eq(0).click() would probably be the same), rather than .click(someFunction); someFunction(); (i.e. passing a function to click and then calling that function directly). Using change seems a bit cleaner and both patterns are present throughout the Trac codebase.

I suppose it all depends on whether there's a side-effect or not, and if yes, if the side-effect it predictable, or not (e.g. what would happen if there would be other event handlers?). The second pattern ensure you have a strict control over what you force during this initial call.

The strict control sounds better for long-term maintainability. Thanks for giving it some thought and explaining the trade-off.

I'll put further comments on #11056, but just wanted to follow-up on this earlier discussion here.

comment:13 Changed 21 months ago by Ryan J Ollos <ryan.j.ollos@…>

  • Description modified (diff)

comment:14 Changed 9 months ago by rjollos

  • Milestone changed from next-dev-1.1.x to 1.1.3

comment:15 Changed 8 months ago by rjollos

  • Milestone changed from 1.1.3 to 1.1.2
  • Owner changed from cboos to rjollos

comment:16 Changed 5 months ago by rjollos

  • Description modified (diff)

comment:17 Changed 4 months ago by rjollos

  • Description modified (diff)

comment:18 in reply to: ↑ description ; follow-up: Changed 4 months ago by rjollos

  • API Changes modified (diff)
  • Release Notes modified (diff)

Replying to Ryan J Ollos <ryan.j.ollos@…>:

  • Addition of a Select All checkbox in the table header.

Proposed changes in log:rjollos.git:t10994.2.

I've prefixed variable names containing jQuery objects with $ (SO:553734/121694). I find the convention to be useful and fairly common in practice, but it should be discussed as to whether it might be adopted in TracDev/CodingStyle#JavaScript.

comment:19 Changed 4 months ago by rjollos

  • API Changes modified (diff)
  • Release Notes modified (diff)

Change from comment:18 committed to trunk in [12954:12955]. Next I'll prepare changes to integrate the drag and drop functionality of the AdminEnumListPlugin.

comment:20 in reply to: ↑ 18 Changed 4 months ago by rjollos

Replying to rjollos:

I've prefixed variable names containing jQuery objects with $ (SO:553734/121694). I find the convention to be useful and fairly common in practice, but it should be discussed as to whether it might be adopted in TracDev/CodingStyle#JavaScript.

I noticed two small things:

  • I didn't follow the above convention everywhere in the changeset.
  • this is a jQuery object, so we don't need to wrap it in $(). In fact, var $table = this; isn't needed at all, and is just present for code clarity.
    • trac/htdocs/js/trac.js

      diff --git a/trac/htdocs/js/trac.js b/trac/htdocs/js/trac.js
      index 3029392..538334c 100644
      a b  
      2424
      2525  // Add a Select All group toggler to each thead in the table.
      2626  $.fn.addSelectAllTogglers = function() {
      27     var $table = $(this);
       27    var $table = this;
      2828    if ($("tr td.sel", $table).length > 0) {
      2929      $("tr th.sel", $table).append(
      3030        $('<input type="checkbox" name="toggle_group" />').attr({
       
      3636        })
      3737      );
      3838      $("tr td.sel", $table).click(function() {
      39         var tbody = $(this).closest("tbody");
      40         var checkboxes = $("tr td.sel input", tbody);
      41         var numSelected = checkboxes.filter(":checked").length;
       39        var $tbody = $(this).closest("tbody");
       40        var $checkboxes = $("tr td.sel input", $tbody);
       41        var numSelected = $checkboxes.filter(":checked").length;
      4242        var noneSelected = numSelected === 0;
      43         var allSelected = numSelected === checkboxes.length;
      44         $("tr th.sel input", tbody.prev())
       43        var allSelected = numSelected === $checkboxes.length;
       44        $("tr th.sel input", $tbody.prev())
      4545          .prop({"checked": allSelected,
      4646                 "indeterminate": !(noneSelected || allSelected)});
      4747      });

comment:21 follow-up: Changed 4 months ago by rjollos

Coding guideline added in TracDev/CodingStyle, but always open for further discussion of course.

comment:22 in reply to: ↑ 21 Changed 4 months ago by cboos

Replying to rjollos:

Coding guideline added in TracDev/CodingStyle, but always open for further discussion of course.

I agree, it's useful.

comment:23 Changed 4 months ago by rjollos

  • Description modified (diff)

Created tickets #11682 and #11683 for work that won't be completed for 1.1.2.

comment:24 Changed 4 months ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to closed

Changes from comment:20 committed in [12963].

comment:25 Changed 4 months ago by rjollos

  • API Changes modified (diff)

Renamed function in [13008], in consideration of changes that will be made in #11417.

Modify Ticket

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