Edgewall Software

Opened 11 years ago

Last modified 10 years ago

#10994 closed enhancement

Add drag and drop and other dynamic behavior to the admin panels containing enum lists — at Version 19

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

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

API Changes:

The function addSelectAllTogglers 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.

Internal Changes:

Description (last modified by Ryan J Ollos)

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.
    • 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.

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

Change History (20)

comment:1 by Remy Blank, 11 years ago

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 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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 by lkraav <leho@…>, 11 years ago

Cc: leho@… added

by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Attachment: t10994-r11508-1.patch added

Patch against r11508 of the trunk.

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

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.

in reply to:  4 ; comment:5 by Christian Boos, 11 years ago

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.

in reply to:  5 comment:6 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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 by Christian Boos, 11 years ago

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

comment:8 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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 by Christian Boos, 11 years ago

Milestone: 1.0.2next-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 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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.

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

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.

in reply to:  5 comment:12 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Description: modified (diff)

comment:14 by Ryan J Ollos, 10 years ago

Milestone: next-dev-1.1.x1.1.3

comment:15 by Ryan J Ollos, 10 years ago

Milestone: 1.1.31.1.2
Owner: changed from Christian Boos to Ryan J Ollos

comment:16 by Ryan J Ollos, 10 years ago

Description: modified (diff)

comment:17 by Ryan J Ollos, 10 years ago

Description: modified (diff)

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

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 by Ryan J Ollos, 10 years ago

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.

Note: See TracTickets for help on using tickets.