Edgewall Software
Modify

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#11056 closed enhancement (fixed)

Disable submit buttons if required items are not selected

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

Submit buttons are disabled if the required items are not selected.

API Changes:

To add a datepicker or datetimepicker to an element, add the trac-datepicker and trac-datetimepicker classes respectively, and add jQuery UI and add-ons to the page using Chrome(self.env).add_jquery_ui(req) in process_request.

To disable a submit button associated with a set of checkboxes or a file selector, add the trac-disable class to the submit button and add the trac-disable-determinant class to each checkbox or the file element.

In both cases there is no need to add any specific JavaScript code to the page.

Description (last modified by Ryan J Ollos <ryan.j.ollos@…>)

A feature initially discussed in #10994 is to disable Remove selected items and Add attachment buttons if no items have been selected - table entries for the former and a file for the latter. The feature might be implemented for some additional buttons while working on this ticket. The initial patch can be seen in ticket:10994:t10994-r11508-1.patch. I'm targeting this to 1.0.2 per comment:9:ticket:10994.

Attachments (1)

t11056-r11682-1.patch (10.2 KB) - added by Ryan J Ollos <ryan.j.ollos@…> 3 years ago.
Patch against r11682 of the trunk.

Download all attachments as: .zip

Change History (27)

Changed 3 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: t11056-r11682-1.patch added

Patch against r11682 of the trunk.

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

t11056-r11682-1.patch applies behavior to the Add attachment (/attachment/*), Change tickets (/query), Install (/admin/general/plugin) and Remove selected items (/admin/ticket/*, /admin/versioncontrol/repository, /admin/general/perm) buttons.

One particular behavior worth pointing out is that by clicking on the labels for the Subject or Group all of the checkboxes in the row are toggled. This is a nice convenience, but unfortunately I can't see a way to make it more obvious to the user.

There are some other places that similar behavior could be implemented. I've opted not to include these in the patch because I couldn't convince myself that they would necessarily improve usability, but I'm hoping to hear additional opinions:

  • /newticket: Disable Create ticket until at least one character has been entered in the Summary (or disable if summary is greater than max_description_size).
  • /search: Disable Search until at least min_query_length characters have been entered.
  • /milestone?action=new: Disable Add milestone until at least one character has been entered for the Name of the milestone field.

The general criteria I took when deciding where to implement the button disabling is to disable a button if the user could be redirected to an error page by not selecting an item before submitting.

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

Description: modified (diff)
Summary: Disable 'Apply Changes' and 'Add Attachment' button if no items selectedDisable submit buttons if required items are not selected

comment:3 Changed 3 years ago by Remy Blank

Owner: set to Remy Blank
Status: newassigned

Looks nice. I'll review and apply the current patch.

comment:4 Changed 3 years ago by Christian Boos

I'm a bit concerned about the increasing complexity of the Javascript, which changes like this.

For example in this patch, there are two very specific selectors for each of the invocation of which.disableIfNoneChecked(what). Not only one has to find the proper selector expressions, but they have to be maintained whenever the structure changes (either which or what).

So I wonder if we couldn't find a better way to encapsulate this, kind of standard "behaviors" for those forms (e.g. a trac-choose class on the which side, a trac-choose-option class for the input from which one must be chosen (or a "negative" one), and have the disableIfNoneChecked() behavior applied to all of these, without having to care for how the individual case is placed or named within each page.

In short, I think we should find a way to improve encapsulation, pack together the structure and its behavior.

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

That sounds interesting, but I'm probably not grasping the plan yet. It sounds like it might be a way to satisfy my aim for some lightweight generic code in comment:3:ticket:11027 though.

Would it look something like this?:

  1. Add trac-disable class to the Remove selected items button.
  2. Add trac-disable-unchecked class to the checkboxes.
  3. Add the following trac.js:
    $("input:submit.trac-disable").disableIfNoneChecked(
      "input:checkbox.trac-disable-unchecked");
    
  4. Modify $.fn.disableIfNoneChecked to take a selector string as input rather than a wrapped set.

comment:6 Changed 3 years ago by Christian Boos

Well, I don't have a plan yet, just trying to shape things as we go… Something good will hopefully emerge ;-)

And what you proposed is already much better I think.

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

I'll work up a new patch to serve as a point of discussion. It will probably be a few days before I can spend the required time on it though.

#11027 may be another good use-case to help with shaping things since the patch is fairly simple.

comment:8 in reply to:  1 Changed 3 years ago by Ryan J Ollos <ryan.j.ollos@…>

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

One particular behavior worth pointing out is that by clicking on the labels for the Subject or Group all of the checkboxes in the row are toggled. This is a nice convenience, but unfortunately I can't see a way to make it more obvious to the user.

I probably got the idea for this from th:QueryUiAssistPlugin, and I think it would be worthwhile to implement the feature more widely in Trac such as on the custom query filter labels.

comment:9 Changed 3 years ago by Ryan J Ollos

Owner: changed from Remy Blank to Ryan J Ollos

comment:10 Changed 3 years ago by Ryan J Ollos

Milestone: 1.0.2next-dev-1.1.x

comment:11 Changed 3 years ago by Ryan J Ollos

Milestone: next-dev-1.1.x1.1.2

comment:12 Changed 3 years ago by Ryan J Ollos

I've been working on simplifying the selectors, with the idea that the jQuery code would go in layout.html. A similar, and much simpler scenario, would involve moving the datepicker and datetimepicker calls to layout.html, so I made an attempt at that change first: log:rjollos.git:t11056. How does that look?

I was a bit surprised by the need to test the length of the jQuery object before calling datepicker and datetimepicker. I haven't looked at the source, but I wonder why the behavior is different than other jQuery functions such as focus?:

> $().focus()
[]
> $().datetime()
TypeError: Object [object Object] has no method 'datetime'
Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:13 in reply to:  12 Changed 3 years ago by Jun Omae

I've been working on simplifying the selectors, with the idea that the jQuery code would go in layout.html. A much simpler scenario would be involve moving the datepicker and datetimepicker calls to layout.html, so I made an attempt at that change first: log:rjollos.git:t11056. How does that look?

Looks good. However, I like .trac-datepicker and .trac-datetimepicker rather than .trac-date and .trac-datetime. We might use the classes like this in the future.

   <span class="trac-datetime" data-trac-datetime="2013-12-21T15:22:42Z">42 minutes ago</span>

I was a bit surprised by the need to test the length of the jQuery object before calling datepicker and datetimepicker. I haven't looked at the source, but I wonder why the behavior is different than other jQuery functions such as focus?:

$.datepicker and $.datetimepicker functions require jquery-ui.js and jquery-ui-addons.js. If Chrome.add_jquery_ui is not called, these functions are not available.

  • trac/templates/layout.html

    diff --git a/trac/templates/layout.html b/trac/templates/layout.html
    index b1fe39f..5be3b34 100644
    a b  
    4747        </py:if>
    4848        $(".trac-autofocus").focus();
    4949        $(".trac-target-new").attr("target", "_blank");
    50         if ($(".trac-date").length) $(".trac-date").datepicker();
    51         if ($(".trac-datetime").length) $(".trac-datetime").datetimepicker();
     50        if ($.ui) { /* is jquery-ui added? */
     51          $(".trac-date").datepicker();
     52          $(".trac-datetime").datetimepicker();
     53        }
    5254        setTimeout(function() { $(".trac-scroll").scrollToTop() }, 1);
    5355      });
    5456    </script>
Last edited 3 years ago by Jun Omae (previous) (diff)

comment:14 Changed 3 years ago by Ryan J Ollos

Latest changes can be found in log:rjollos.git:t11056.

  • Included Jun's changes from comment:13. Thanks!
  • Group toggling on permissions page:
    • I could imagine some refinement to this behavior, but it's not clear to me yet.
    • The code to disable selection of text is from SO:2700029/121694 and a function could be added to trac.js.
  • I'm not sure that I like the naming of the trac-disable / trac-disable-determinant classes, so I will continue to give that some thought.

Tested with Chrome 31 / Firefox / Opera 12.16 on Ubuntu 13.04.

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

comment:15 Changed 3 years ago by Ryan J Ollos

Fixed an issue and cleaned up commit history in log:rjollos.git:t11056.2.

comment:16 in reply to:  14 Changed 3 years ago by Ryan J Ollos

Replying to rjollos:

  • The code to disable selection of text is from SO:2700029/121694 and a function could be added to trac.js.

If that isn't done though, the multiple calls to css and attr could be combined.

comment:17 Changed 3 years ago by Ryan J Ollos

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

Committed to trunk in [12373:12374]. Minor change committed to 1.0-stable in [12375] and merged to trunk in [12376].

comment:18 in reply to:  14 Changed 3 years ago by Ryan J Ollos

Replying to rjollos:

  • Group toggling on permissions page:
    • I could imagine some refinement to this behavior, but it's not clear to me yet.
    • The code to disable selection of text is from SO:2700029/121694 and a function could be added to trac.js.

These changes have been moved to #11417.

comment:19 Changed 3 years ago by Ryan J Ollos

I'm thinking that we should add a tooltip: You must select at least one item in the table. Previously the batch modification form would display next to the Change tickets button: You must select at least one ticket.

comment:20 Changed 3 years ago by Ryan J Ollos

Also, it seems like we could just remove the disableSubmit call from query.js and add the trac-disable, trac-disable-determinant classes where necessary.

comment:21 Changed 3 years ago by Ryan J Ollos

Proposed changes can be found in log:rjollos.git:t11056.3. Moving the validation out of submit in the batch modification form will make it easier to implement #10138 by avoiding multiple submit handlers (comment:21:ticket:10138).

comment:22 Changed 3 years ago by Ryan J Ollos

The string in [22aa2281/rjollos.git] needs to be wrapped for translation. I'll fix that before committing the changes.

comment:23 Changed 3 years ago by Ryan J Ollos

Changes from comment:21 committed in [12450:12451].

comment:24 Changed 2 years ago by Ryan J Ollos

In [12373#file1] the trac-datetimepicker class is only added if readonly is False. It looks like it would be better to modify the selectors so that the datepicker and datetimepicker aren't added if the readonly attribute is present:

  • trac/templates/layout.html

    diff --git a/trac/templates/layout.html b/trac/templates/layout.html
    index 894834f..0d27f8b 100644
    a b  
    6060        $(".trac-autofocus").focus();
    6161        $(".trac-target-new").attr("target", "_blank");
    6262        if ($.ui) { /* is jquery-ui added? */
    63           $(".trac-datepicker").datepicker();
    64           $(".trac-datetimepicker").datetimepicker();
     63          $(".trac-datepicker:not([readonly])").datepicker();
     64          $(".trac-datetimepicker:not([readonly])").datetimepicker();
    6565        }
    6666        $(".trac-disable").disableSubmit(".trac-disable-determinant");
    6767        setTimeout(function() { $(".trac-scroll").scrollToTop() }, 1);
  • trac/ticket/templates/milestone_edit_form.html

    diff --git a/trac/ticket/templates/milestone_edit_form.html b/trac/ticket/templa
    index 5e85c81..372bfde 100644
    a b  
    6262          </label>
    6363          <label>
    6464            <input type="text" id="duedate" name="duedate"
    65                    class="${'trac-datetimepicker' if not readonly else None}"
    66                    size="${len(datetime_hint)}"
     65                   class="trac-datetimepicker" size="${len(datetime_hint)}"
    6766                   value="${format_datetime(milestone.due or default_due)}"
    6867                   readonly="$readonly"
    6968                   title="${_('Format: %(datehint)s', datehint=datetime_hint)}"
     
    7877          </label>
    7978          <label>
    8079            <input type="text" id="completeddate" name="completeddate"
    81                    class="${'trac-datetimepicker' if not readonly else None}"
    82                    size="${len(datetime_hint)}"
     80                   class="trac-datetimepicker" size="${len(datetime_hint)}"
    8381                   value="${format_datetime(milestone.completed)}"
    8482                   readonly="$readonly"
    8583                   title="${_('Format: %(datehint)s', datehint=datetime_hint)}"

comment:25 Changed 2 years ago by Ryan J Ollos

We should maybe capture disabled elements as well: .trac-datepicker:not([readonly]):not([disabled]) (untested). does not seem to be needed.

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

comment:26 Changed 2 years ago by Ryan J Ollos

Change from comment:24 committed in [12867].

Modify Ticket

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