JavaScript attachment deletions
jpetso - May 27, 2007 - 09:10
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | upload.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | jpetso |
| Status: | patch (code needs work) |
Description
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-)
| Attachment | Size |
|---|---|
| upload-js-delete.patch | 7.14 KB |
#1
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.
#2
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.
#3
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.
#4
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.
#5
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.)
#6
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!
#7
subscribing, sounds like a cool change
#8
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.
#9
Tried this on IE6 (as requested) and it works a treat. Rather nice actually, well done guys!
#10
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.
#11
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).
#12
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.
#13
w00t! We can now do t() in JavaScript! Saves 3 lines, including an ugly PHP->JavaScript data transfer. Updated patch attached, please review.
#14
This is a worthwhile usability improvement.
The approach seems generally well done. I haven't tested. Some comments on the code:
1.
Drupal.getFileDeleteDatashould 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
initFileDeleteandDrupal.updateFileDeletemethods into methods of thefileDeleteDataobject.3. Several variables are used without being declared.
4. This seems a bit awkward:
<?php+ $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?
#15
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.