DROP Task: Alter flexifilter administration interface to use tabledrag API.
arhip - January 11, 2008 - 02:14
| Project: | Flexifilter |
| Version: | 6.x-1.1-rc1 |
| Component: | User interface - Admin pages |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Description
Hi.
I think Edit flexifilter form is too crowded when there are many components, especially for nested components. I suggest to use tabledrag instead of fieldsets, to improve user friendliness.
#1
Good idea; the administration pages will likely see reworking in the 1.2 version if not the 1.1 version. As always, patches are welcome :) .
#2
This is now a DROP task.
#3
(Forgot about the no-numbering rule)
#4
Inital patch... doesn't add Drag 'n Drop.
#5
Another update... has lots of code that doesn't yet work
#6
Now with awesomeness out of the box
#7
Latest update
#8
Getting this:
Warning: Call-time pass-by-reference has been deprecated - argument passed by value; If you would like to pass it by reference, modify the declaration of form_get_cache(). If you would like to enable call-time pass-by-reference, you can set allow_call_time_pass_reference to true in your INI file. However, future versions may not support this any longer. in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 762<?php$form = form_get_cache($form_build_id, &$form_state);
?>
<?php$form = form_get_cache($form_build_id, $form_state);
?>
#9
Firstly, I cannot edit things using Firefox2. I can rearrange components, but clicking on one doesn't yield a way to edit it.
Secondly, saving a flexifilter gives a bunch of PHP notices:
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 212.* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 212.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 212.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 212.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 212.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 212.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 212.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 212.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 219.
* notice: Undefined index: class in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 220.
* notice: Undefined index: in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 220.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 219.
* notice: Undefined index: class in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 220.
* notice: Undefined index: in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 220.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 219.
* notice: Undefined index: class in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 220.
* notice: Undefined index: in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 220.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 219.
* notice: Undefined index: class in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 220.
* notice: Undefined index: in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 220.
* notice: Undefined index: id_prefix in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 186.
* notice: Undefined index: id_next in E:\HTML\Apache\users\drupal60_dev_clean\modules\flexifilter\flexifilter.admin.inc on line 187.
#10
I have concerns with the JavaScript file in this patch. The JS code is conveniently reproduced below due to the lack of the actual existence of the file as of now, because the JS file is still part of the patch.
(function($) {$(function() {
$('#flexifilter-components tbody tr').each(function() {
$(this).find('td:first').bind('click', function() {
component_id = $(this).parent().find('td:last .flexifilter-cid').val();
$('#flexifilter-components tbody tr').removeClass('flexifilter-component-highlighted');
$.scrollTo($('#flexifilter-component-settings').offset().top - 10, 'slow');
$('#flexifilter-component-settings').load(Drupal.settings.basePath +'admin/build/flexifilters/js/component_edit/'+ Drupal.settings.flexifilterFid +'/'+ component_id, function() {
Drupal.attachBehaviors($(this));
$(this).css('background-color', '#FCFF97');
$(this).animate({ 'background-color': '#ffffff' }, 'slow');
});
return false;
});
});
});
// This is only when flexifilter.inc is included
Drupal.theme.prototype.tableDragChangedWarning = function() {};
})(jQuery);
Let's start with line 2.
$(function() {/*...*/})is certainly a very compact way to state$(document).ready(function() {/*...*/}), but it does not offer as much clarity. It's not a severe problem, though.The biggest problem is this:
component_id = $(this).parent().find('td:last .flexifilter-cid').val();It should be this:
var componentId = $(this).parent().find('td:last .flexifilter-cid').val();The reason to use
varis because in JavaScript if you don't, and if a global variable has the same name, it would be overwritten. Usingvarwould indicate that it's intended to be local. Not only that, you should never leave outvarfor declaring local variables because it's costly. It's an expensive operation since the JavaScript engine would have to go through the trouble of looking for a variable with the same name. (A more technically correct way to put it is that it would search up the whole scoping chain until it hits the global namespace if there aren't any variables with the same name.)So never leave out
varunless it's intended. I suggestedcomponentIdinstead ofcomponent_idbecause in JavaScript, camelCasing is the practice, rather than underscore_separation.Lastly, it would be best to consolidate the following two lines:
$(this).css('background-color', '#FCFF97');$(this).animate({ 'background-color': '#ffffff' }, 'slow');
Into one line:
$(this).css('background-color', '#FCFF97').animate({ 'background-color': '#ffffff' }, 'slow');Not only would this save space and eliminate visual redundancy, it would also avoid calling
$(this)twice, speeding up the performance (a tiny bit).I'm happy to see the
(function($) {/*...*/})(jQuery);wrapping. This ensures library compatibility.#11
Word on the street says dmitrig01 is still working on this locally.
#12
#13
subscribe
#14
Subscribe