Review: Needs Fixing == Looks and behavior of feature * Big borders in Webkit for custom columns box (fieldset has borders)
* Why are [SEARCH] and [CLEAR] gone off to the right? They're the most important buttons in the filter view * Style of custom columns group completely different than other groups (group by, extended filters, filters) * Why don't the FILTERS group use the same implementation as the other groups (group by & extended filters)? * Too much space between the [-] button and the field's label, fix (colspans?) == Potential improvements (to discuss) * Rename [Custom Columns] to [Hide Columns] * Why not a button on the column itself (like a [x] to close the column) and replace [CUSTOM COLUMNS] by [RESET COLUMNS] or something like that, and it would re-enable all columns? == Peripheral stuff to maybe fix * What's with the [Actions] in the dropdown? Can we have actions there still? Why not remove it? == Tech I'm sure it took you some time to develop this, you can create more than one big commit. More smaller commits is usually (though not always) better: better commit messages, more self-contained, easier to review, ... (that means you don't have to create a single commit with fixes to all remarks you think are worth fixing for instance, though you don't have to create a commit for each either). Commits are cheap. revid:[email protected], search.py * All_values does not match naming conventions. `all` should be lowercase * stray whitespace at end of `for key, val in v.items():` line (and in a bunch of others places in that commit and the next) * get_domain extraction is nice, but there is no need to accumulate: all cases are exclusive, just return the values already, there is no need for the tempdomain temporary variable * Within get_domain's len == 3 and len == 4, tuple unpacking would probably make function more readable. Also, string formats and double quotes (to avoid having to escape single quote in value being created). * the tmp_domain accumulator should be a list which is joined at the end. * surely there is a better way than creating sequences of [stuff] and then doing string replacement of ][, [ and ] no? revid:[email protected], search.js * Wasn't always done before, but try to prefix names of variables containing jQuery objects with $ to differentiate from raw DOM nodes or nodelists (gotten from native DOM methods or MochiKit): filter_opt_tbl or the old filter_table should be $filter_opt_tbl and $filter_table. Several instances of this in the code/patch * Chain jquery methods (maybe not too much, but it's usually clear and readable) for example lines jQuery('label#filterlabel').text(jQuery('select.filter_fields_and option:selected').text()) jQuery('label#filterlabel').attr('value', jQuery(elem).val()) can be chained, no need to fetch 'label#filterlabel' twice. Same under with new_tr manipulations. Just be careful with line breaks. * If you have IDs, you shouldn't need element name prefixes. IDs should be unique in the page. So `#filterlabel` not `label#filterlabel` * don't define variables twice (`position_tr` has a second `var` in condition, also it should be $-prefixed as it's a jQuery object) * Also try to use consistent indentation (all spaces, 4 spaces per level), to remove stray line-ending whitespace (use bzr cdiff --check-style to verify) and don't forget the `;` at the end of JS lines (in $curr_body.find.each callback, there are at least 4 lines with no `;`) * Try to avoid nesting too much, and use return guards: function(i, v) { var theValue = jQuery(v).text(); if (theValue == jQuery('select.filter_fields_and option:selected').text()){ index_row = i new_tr.find('select.expr').hide() new_tr.find('label#filterlabel').hide() new_tr.find('label.and_or').remove() var select_andor = jQuery('<label>', {'class': 'and_or'}).text('OR'); select_andor.insertBefore(new_tr.find('input.qstring')); } } should be: function(i, v) { var theValue = jQuery(v).text(); if (theValue != jQuery('select.filter_fields_and option:selected').text()) { return; } index_row = i new_tr.find('select.expr').hide() new_tr.find('label#filterlabel').hide() new_tr.find('label.and_or').remove() var select_andor = jQuery('<label>', {'class': 'and_or'}).text('OR'); select_andor.insertBefore(new_tr.find('input.qstring')); } } and there are unneeded temporary variables again. * Be careful with globals, check if your editor/IDE can check them for you, also maybe enable Firefox's Strict mode in firebug (in switch_searchView there is a `i` used without `var` in a loop, so it's a global) * Why isn't tbodys a jQuery object? * In e.g. remove_filter_row you unwrap jquery objects, select one and rewrap immediately. Should be no need to e.g. `jQuery($paren.children('tbody')[t.index()-1])` can be written `$paren.children('tbody').eq(t.index() - 1)` no? * Use the right methods e.g. rather than `jQuery(this).find('> .filter_row_class');` is it not possible to use ` jQuery(this).children('.filter_row_class');`? That would do the same thing no? It's more readable and probably jQuery is better optimized to boot. * Also don't split selectors when no need to, a few lines lower there is `var children = jQuery('#filter_option_table tbody').find('> .filter_row_class')` why is it not `var children = jQuery('#filter_option_table tbody > .filter_row_class')`? * Don't needlessly rewrap in jQuery either: if $curr_body is a jQuery object, $curr_body.find('> .filter_row_class').eq(trs_keys[index]) is a jQuery object, no need to call jQuery() on it revid:[email protected], search.js * Use litteral collections, `[]` not `new Array()` * Also why is custom_columns a global variable? * jQuery objects have a length, no need for `jQuery('#custom_columns input:not(:checked)').get().length` * For creating c_columns, why not use jQuery().map().get()? * Also why does first condition return custom_columns as a string and second one as an array? * And why initialize custom_columns above function when that initialization is never used? same revision, listgrid.py * Why reindent the whole function (and add one level) instead of just using a guard and returning early? * Also use `not in` operator, `a not in b` is clearer than `not a in b`. * Not new code, but dict.get returns None by default, so in attrs.get('widget', False) the False is redundant since we're just using for boolean test. Just `attrs.get('widget')` is enough Same revision, viewform.mako * % if screen.view_type == 'tree': % if screen.widget: why two different tests, why not just one with an `and` between? -- https://code.launchpad.net/~openerp-dev/openobject-client-web/improved-custom-filters/+merge/32975 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-client-web/improved-custom-filters. _______________________________________________ Mailing list: https://launchpad.net/~openerp-dev-gtk Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-dev-gtk More help : https://help.launchpad.net/ListHelp
