Modify

Opened 22 months ago

Closed 19 months ago

Last modified 16 months ago

#10695 closed enhancement (fixed)

Enable the 'Remove selected items' button only when a checkbox has been selected, and add a 'select all' checkbox in the table header

Reported by: rjollos Owned by: rjollos
Priority: normal Component: AdminEnumListPlugin
Severity: normal Keywords:
Cc: nonplus, jun66j5 Trac Release:

Description (last modified by rjollos)

  • Enable the Remove selected items button only when a checkbox has been selected.
  • Add a select all checkbox in the table header for the column that contains the checkboxes.

Attachments (0)

Change History (13)

comment:1 Changed 22 months ago by rjollos

  • Description modified (diff)
  • Status changed from new to assigned
  • Summary changed from Enabled the 'Remove selected items' button only when a checkbox has been selected and add a 'select all' checkbox in the table header to Enable the 'Remove selected items' button only when a checkbox has been selected, and add a 'select all' checkbox in the table header

comment:2 Changed 21 months ago by rjollos

(In [12461]) Refs #10695:

  • The Remove selected items button is enabled only when a checkbox has been selected for at least one of the items.
  • Added a tri-state Select all checkbox to the table header.
  • The Revert changes button is now enabled when a radio button has been selected.
  • Refactoring:
    • Extracted some selectors into variables.
    • Renamed some variables.

comment:3 Changed 21 months ago by rjollos

(In [12463]) Refs #10657, #10695:

  • closest doesn't exist until jQuery 1.3, so its use was replaced with parents.
  • prop is recommended for setting the DOM element statesdisabled and checked in jQuery 1.6+. prop is aliased to attr in version of jQuery that it doesn't exist.


In Trac 0.11.0 (jQuery 1.2.3) the indeterminate state of the checkbox is not seen. It may not be supported in earlier versions of jQuery.

comment:4 Changed 21 months ago by rjollos

I've committed all the work that I have planned for this plugin. I'll wait a little while to see if I get any testing feedback, then change the version from 2.0dev to 2.0 and create a tag.

comment:5 follow-up: Changed 21 months ago by jun66j5

I tried the "select all" checkbox feature. But I got TracError: severity all does not exist. if submitting with checked "select all" checkbox on Firefox 17.

The following patch avoids wrongly sending sel=all in the request data.

  • adminenumlistplugin/htdocs/adminenumlist.js

     
    4141    }); 
    4242 
    4343    // Add a checkbox for toggling the entire column of checkboxes 
    44     var $group_checkbox = $('#enumtable thead th.sel').html('<input type="checkbox" name="sel" value="all"/>').children(); 
     44    var $group_checkbox = $('#enumtable thead th.sel').html('<input type="checkbox" value="all"/>').children(); 
    4545    $group_checkbox.click(function() { 
    4646        $remove_checkboxes.prop('checked', this.checked); 
    4747        $remove_button.prop('disabled', !this.checked); 

comment:6 in reply to: ↑ 5 Changed 21 months ago by rjollos

Replying to jun66j5:

I tried the "select all" checkbox feature. But I got TracError: severity all does not exist. if submitting with checked "select all" checkbox on Firefox 17.

I remember being concerned about having the name attribute on that element, but I can't remember or see now why I thought it was necessary. It looks like maybe we don't need the value attribute either.

Thanks for the patch. I granted you commit access in case you'd like to push it directly.

comment:7 Changed 21 months ago by rjollos

I'll get this committed later this evening, and also spotted a few other minor issues that I'll fix. Recently, I've been working on t:#9609, t:#10992 and #10745, which have a lot of overlap with this ticket.

Jun: consider my invitation to commit to be open-ended, meaning, if you find any issues in the future, please feel free to commit directly.

comment:8 Changed 21 months ago by jun66j5

(In [12509]) refs #10695:

  • Avoids sending the value of "select all" checkbox in the request data if checked

comment:9 Changed 21 months ago by jun66j5

Thanks, Ryan. I've committed my patch with your suggestions in r12509.

comment:10 Changed 21 months ago by rjollos

Just a side note - I've frequently found myself having to add and then immediately select a bit of HTML. As you'll see from the code, I've been using this pattern .html('<input type="checkbox" />').children(), but thinking there might be a better, more clear way to do this.

comment:11 Changed 19 months ago by rjollos

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

comment:12 Changed 16 months ago by rjollos

With regard to [12461] and [12463], where we used,

if(!$.isFunction($.fn.prop))

and

if(!$.isFunction($.curCSS))

to check for the existence of a function bound to $, I learned today that the following also work,

if (!('prop' in $))
if (!('curCss' in $))

The two lines are untested, but I'm just extrapolating on what I found to work in [13192]. Using the in keyword seems like it might lead to slightly clearer code (see also SO:7174748).

I'm not suggesting to change the code for AdminEnumListPlugin, just discussing some various ways of doing things.

comment:13 Changed 16 months ago by jun66j5

It seems that the two lines work.

However, I normally use $.fn.name to check for the existences of jQuery object methods.

    if ($.fn.prop) { /* 1.6+ */ }

If the existence of the utility methods, e.g. $.contains, use $.name.

    if ($.contains) { /* 1.4+ */ }

ref. http://learn.jquery.com/plugins/basic-plugin-creation/.

I don't know that whether $.name work to check for the object methods for any jQuery versions.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.