Edgewall Software
Modify

Opened 16 months ago

Last modified 3 weeks ago

#10994 assigned enhancement

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:
API Changes:

Description (last modified by Ryan J Ollos <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 list can be sorted by drag and drop of the items.
  • The column of select elements is hidden.
  • A Revert changes button is added. The button is disabled until a change is made.
  • 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).

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@…> 16 months ago.
Patch against r11508 of the trunk.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 16 months 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 16 months 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 16 months ago by lkraav <leho@…>

  • Cc leho@… added

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

Patch against r11508 of the trunk.

comment:4 follow-up: Changed 16 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 16 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 15 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 15 months ago by cboos

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

comment:8 Changed 15 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 15 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 15 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 15 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 15 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 14 months ago by Ryan J Ollos <ryan.j.ollos@…>

  • Description modified (diff)

comment:14 Changed 2 months ago by rjollos

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

comment:15 Changed 3 weeks ago by rjollos

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned The owner will remain rjollos.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from rjollos to the specified user.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.