This patch implements the often-requested usability improvement for upload.module's file attachment listing, by replacing the "Delete" checkbox with a "Delete" button. It doesn't delete the file on-the-fly, but just sets the value of the (hidden) checkbox, hides the other editing widgets when Delete is clicked, and displays a nice helpful text instead.

Features:

  • "Delete" button changes to "Cancel" when it has been clicked.
  • Remembers marked-for-deletion state even after the list has been AJAX-reloaded with a new attachment.
  • Flexible enough to allow other modules to use this functionality as well (e.g. filefield/imagefield). The only thing they need to have in common is a Delete checkbox, all other stuff can look and be structured completely different.
  • Still works without JavaScript, of course.
  • Blingy fade effect! x-)
CommentFileSizeAuthor
#13 upload-js-delete-try8.patch7.82 KBjpetso
#12 upload-js-delete-try7.patch8.35 KBjpetso
#6 upload-js-delete-try6.patch8.41 KBjpetso
#5 upload-js-delete-try5.patch7.68 KBjpetso
#4 upload-js-delete-try4.patch7.71 KBjpetso
#3 upload-js-delete-try3.patch7.7 KBjpetso
#1 upload-js-delete-try2.patch7.14 KBjpetso
upload-js-delete.patch7.14 KBjpetso

Comments

jpetso’s picture

jpetso commented
StatusFileSize
new upload-js-delete-try2.patch7.14 KB

Wording consistency: I wanted to use "...until you save this post" instead of "...unless you save this post", but somehow missed it. Fixed in this reroll of the patch.

joshk’s picture

joshk commented

This applies clean and works as advertised. Usability ++

We may want to tinker with the language here, but it works well. I would think UnConeD may want to review your javascript (I have a feeling it might be implemented with less code), but the theory of what's going on here is sound.

jpetso’s picture

jpetso commented
StatusFileSize
new upload-js-delete-try3.patch7.7 KB

Drupal.uploadAutoAttach() is called at the end of both the 'if' and the 'else' branch, and the previous version of my patch only extended the 'else' branch with restoring the previous 'delete' status. Which does not come with any bugs, because the 'if' branch only covers the case when no item was uploaded before, so nothing needs to be restored either way.

Nevertheless, with regards to readability, I think it might be a good idea to extract Drupal.uploadAutoAttach() out of both if/else branches and place it after the condition, together with the 'delete' status restore statement. So, here is a new version of the patch, functionally equivalent to the previous one.

jpetso’s picture

jpetso commented
StatusFileSize
new upload-js-delete-try4.patch7.71 KB

For layouting purposes (I needed to get imagefield to work with this), it seems better to assign the "form-item" class to the button, just as the original Delete checkbox belongs to this class as well. This updated patch just adds the class to the button creation code.

jpetso’s picture

jpetso commented
StatusFileSize
new upload-js-delete-try5.patch7.68 KB

Beginner's mistake: of course, the "xxx.txt is scheduled for deletion." text needs to be done with t(). (Doh!)

Also, update the patch to apply cleanly to current HEAD, as the JavaScript aggregation patch added a few semicolons to upload.js. (They are now also at the end of the new functions.)

jpetso’s picture

jpetso commented
StatusFileSize
new upload-js-delete-try6.patch8.41 KB

Per Steef's suggestion, use a CSS class instead of an inline style="display: none;" for initial hiding of yet hidden stuff. Also, the checkbox can automatically be hidden by assigning it the "js-hide" class. Rename the "-hide" and "-show" classes to "-visible" and "-hidden", which hopefully makes for better understanding. Finally, use fadeIn('fast') instead of plain fadeIn().

Free for another round of reviews!

drewish’s picture

drewish commented

subscribing, sounds like a cool change

jpetso’s picture

jpetso commented

I put up a demo site where you can try how this patch feels like. Log in with "files" as username and "delete" as password, and delete files like there is no tomorrow.

AjK’s picture

AjK commented

Tried this on IE6 (as requested) and it works a treat. Rather nice actually, well done guys!

jpetso’s picture

jpetso commented

For the record, I tested it on Firefox and Konqueror, where it also works just fine. Konqueror of course doesn't do transparency for fadeIn(), but if you don't know how it should look like you won't even notice.

dldege’s picture

dldege commented

Tested and worked as advertised on current Drupal head with no other patches in play.

Very nice feature but I think the status text needs a little work.

"fade.png" is scheduled for deletion. Mind that it will not be deleted until you save this post.

IMO that could just be

"fade.png" is marked for deletion.

The rest is redundant as the whole upload form fieldset contains

Changes made to the attachments are not permanent until you save this post.

No functional issues were found in my testing (FF 2.0 only).

jpetso’s picture

jpetso commented
StatusFileSize
new upload-js-delete-try7.patch8.35 KB

Yes, I think you're right. The additional text came from the patch's filefield origins, where the additional help text above is not included.

Here's an updated patch with the proposed text change.

jpetso’s picture

jpetso commented
StatusFileSize
new upload-js-delete-try8.patch7.82 KB

w00t! We can now do t() in JavaScript! Saves 3 lines, including an ugly PHP->JavaScript data transfer. Updated patch attached, please review.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Status: Needs review » Needs work

This is a worthwhile usability improvement.

The approach seems generally well done. I haven't tested. Some comments on the code:

1. Drupal.getFileDeleteData should create an object. Javascript doesn't really support associative arrays, see this article. Try switching to e.g. 

Drupal.fileDeleteData = function(wrapper) {
  this.wrapper = wrapper;
  ...
}

Then you can create like:

var fileDeleteData = new Drupal.getFileDeleteData(this.id);

Here as well an array is incorrect:

var fileDeleteData = new Array();

Consider

var fileDeleteData = {};

2. Also consider converting the initFileDelete and Drupal.updateFileDelete methods into methods of the fileDeleteData object.

3. Several variables are used without being declared.

4. This seems a bit awkward:


+    $div_visible = '<div class="files-'. $key .'-remove-visible">';
+    $div_hidden  = '<div class="files-'. $key .'-remove-hidden file-delete-hidden">';
+    $div_end     = '</div>';

Can't/shouldn't these wrappers be added to the form elements before theming?

jpetso’s picture

jpetso commented

Thanks for the review, nedjo. I'll look into fixing the points 1, 2 and 3.

As for 4, it would be absolutely no problem to include this in the original form declaration, but I feel that this logically belongs to theming. If others disagree with that, you can most certainly have all those divs in _upload_form(), and it will probably look cleaner.

lilou’s picture

lilou commented
Version: 6.x-dev » 7.x-dev
jpetso’s picture

jpetso commented
Assigned: jpetso » Unassigned

Having rewritten it for FileField v6 (even if that code is mostly gone again, due to *cough* unexpected circumstances), I would think that this should be redone with much less JavaScript and more form state awareness - not the JS should remember the deletion status, but the deleted files should probably not be shown at all and remembered as deleted in the form state only, until the node is submitted.

No motivation to work on this for the time being, let's unassign this from me.

sutharsan’s picture

sutharsan commented
Component: upload.module » usability

Moving this issue to the Usability component for more usability focus.

aspilicious’s picture

aspilicious commented
Component: usability » ajax system
Status: Needs work » Closed (fixed)

We have a remove button! :p