Issue 82042: Melange Surveys: Survey taking and editing JS, additions to CSS.
|
Created:
15 years, 9 months ago by ajaksu Modified:
9 years, 3 months ago Reviewers:
Mario Ferraro CC:
melange-soc-dev_googlegroups.com, thelevybreaks Visibility:
Public. |
Patch Set 1 #Patch Set 2 : Updated #
Total comments: 229
Patch Set 3 : Merged James's code, no fixes from review yet... #Patch Set 4 : Addresses most issues raised #
Total comments: 29
Patch Set 5 : Fixed latest issues #
MessagesTotal messages: 4
Hi guys, for the moment I've put focus on style issues, please don't hate me too much :) There's something like putting "function ()" in a new line if it's a parameter for another function, which is usually personal. I prefer to go that way (and so I wrote in the review), but feel free to discuss it ^__^ Cheers, Mario Ferraro http://codereview.appspot.com/82042/diff/1001/2001 File app/soc/content/css/soc-090602.css (right): http://codereview.appspot.com/82042/diff/1001/2001#newcode648 Line 648: From here to line 671 it lacks indentation http://codereview.appspot.com/82042/diff/1001/2001#newcode660 Line 660: /*margin-left:-100px;*/ Is this still necessary? http://codereview.appspot.com/82042/diff/1001/2001#newcode668 Line 668: div #survey_widget th, div #survey_widget td { Never thought about it, but I think the style you have in lines 687 for multiple selectors in multiple lines it's a good idea, should be adopted by us.. so go for it :) http://codereview.appspot.com/82042/diff/1001/2001#newcode680 Line 680: width:320px; Indentation http://codereview.appspot.com/82042/diff/1001/2001#newcode683 Line 683: div #survey_widget td.formfieldlabel { It seems there are two spaces between widget and td http://codereview.appspot.com/82042/diff/1001/2001#newcode684 Line 684: font-size:medium; Indentation http://codereview.appspot.com/82042/diff/1001/2001#newcode695 Line 695: #survey_widget .quant_radio input, #survey_widget .quant_radio label { See style in line 687 http://codereview.appspot.com/82042/diff/1001/2001#newcode720 Line 720: color:#666666; indentation http://codereview.appspot.com/82042/diff/1001/2001#newcode730 Line 730: .sortable_li {border: solid 1px !important; padding:5px !important;} I suggest to put it in multiple lines http://codereview.appspot.com/82042/diff/1001/2001#newcode733 Line 733: .sortable_li, div #survey_widget fieldset{ See style in line 687 http://codereview.appspot.com/82042/diff/1001/2001#newcode735 Line 735: -webkit-border-radius: 5px; Indentation http://codereview.appspot.com/82042/diff/1001/2001#newcode738 Line 738: Fix new lines and indentation from here to line 744 http://codereview.appspot.com/82042/diff/1001/2001#newcode747 Line 747: .ui-button {outline: 0; margin:0; padding: .4em 1em .5em; Change the whole class and put each statement line per line http://codereview.appspot.com/82042/diff/1001/2001#newcode751 Line 751: .ui-dialog .ui-state-highlight, .ui-dialog .ui-state-error { padding: .3em; } See style in line 687 http://codereview.appspot.com/82042/diff/1001/2001#newcode765 Line 765: padding:0 20px; Indentation http://codereview.appspot.com/82042/diff/1001/2001#newcode783 Line 783: Delete this line :) http://codereview.appspot.com/82042/diff/1001/2001#newcode802 Line 802: margin:-1px 0; Indentation http://codereview.appspot.com/82042/diff/1001/2002 File app/soc/content/js/default-text.js (right): http://codereview.appspot.com/82042/diff/1001/2002#newcode1 Line 1: (function ($) { append a new line for better readability http://codereview.appspot.com/82042/diff/1001/2002#newcode1 Line 1: (function ($) { I suggest to put all your js files to have a prefix of "surveys-", to better separate your code, waiting for the whole "package-like" refactoring before or then :) Also I don't see any "_" to separate words in Javascript files name in the current scripts, so ... well is up to you to keep the style or not :) Add copyright notices to all the files and also authors: /** * @author <a href="mailto:your@mail.box">Name Surname</a> */ http://codereview.appspot.com/82042/diff/1001/2002#newcode2 Line 2: var map = []; append a new line http://codereview.appspot.com/82042/diff/1001/2002#newcode3 Line 3: $.preserveDefaultText = { append a new line http://codereview.appspot.com/82042/diff/1001/2002#newcode5 Line 5: for (var i = 0; i < map.length; i = i + 1) { if it doesn't hurt your scope, use jquery each http://codereview.appspot.com/82042/diff/1001/2002#newcode14 Line 14: }, append a new line http://codereview.appspot.com/82042/diff/1001/2002#newcode16 Line 16: for (var i = 0; i < map.length; i = i + 1) { again, if it doesn't hurt, I suggest to change to jquery each http://codereview.appspot.com/82042/diff/1001/2002#newcode24 Line 24: $.fn.preserveDefaultText = function (text, color) { append a new line http://codereview.appspot.com/82042/diff/1001/2002#newcode27 Line 27: } append a new line http://codereview.appspot.com/82042/diff/1001/2002#newcode31 Line 31: var defaultColor = input.css("color"); append a new line http://codereview.appspot.com/82042/diff/1001/2002#newcode37 Line 37: }; append a new line http://codereview.appspot.com/82042/diff/1001/2002#newcode43 Line 43: } add a semicolon http://codereview.appspot.com/82042/diff/1001/2002#newcode53 Line 53: } add a semicolon http://codereview.appspot.com/82042/diff/1001/2003 File app/soc/content/js/edit_survey.js (right): http://codereview.appspot.com/82042/diff/1001/2003#newcode15 Line 15: Add authors /** * @author <a href="mailto:your@mail.box">Name Surname</a> */ http://codereview.appspot.com/82042/diff/1001/2003#newcode16 Line 16: var DEFAULT_LONG_ANSWER_TEXT = 'Write a Custom Prompt For This Question...'; These variables probably will affect global scope, should be inside the scope where they're needed http://codereview.appspot.com/82042/diff/1001/2003#newcode29 Line 29: $(function () { wrap all stuff into: (function ($) { }(jQuery)); like in default-text.js http://codereview.appspot.com/82042/diff/1001/2003#newcode34 Line 34: var widget = $('div#survey_widget'); append a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode36 Line 36: 'float': 'left', 4 space indentation, not 6 http://codereview.appspot.com/82042/diff/1001/2003#newcode38 Line 38: }); 2 space indentation, not 4 http://codereview.appspot.com/82042/diff/1001/2003#newcode47 Line 47: } append a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode55 Line 55: Fix the comment http://codereview.appspot.com/82042/diff/1001/2003#newcode56 Line 56: var survey_html = $('form').find("#id_survey_html").attr('value'); append a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode78 Line 78: widget.find('.short_answer').each(function () { When anonymous functions are injected as parameters, I suggest (for readability) to have a new line for "function () {" and fix indentations consequently. Something like widget.find().each( function () { blah blah } ); I'll be writing this when encountered, then we'll talk about it eventually :) http://codereview.appspot.com/82042/diff/1001/2003#newcode83 Line 83: widget.find('.long_answer').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode95 Line 95: $('form input, form button, form select').keypress(function (e) { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode105 Line 105: $('a.fetch_answers').click(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode109 Line 109: var query = '?read_only=true&user_results=' + user; be careful not introducing cache issues. If it happens, I suggest to append &_"+(new Date().getTime()) http://codereview.appspot.com/82042/diff/1001/2003#newcode124 Line 124: survey.bind('init', function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode128 Line 128: Comment lines are too long, I suggest to introduce "TODO:" or "FIXME:" for this kind of comments http://codereview.appspot.com/82042/diff/1001/2003#newcode129 Line 129: widget.find('input').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode130 Line 130: if (($(this).val().length < 1 || $(this).val() === DEFAULT_SHORT_ANSWER_TEXT) && Line too long http://codereview.appspot.com/82042/diff/1001/2003#newcode136 Line 136: widget.find('.long_answer').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode137 Line 137: if ($(this).val().length < 1 || $(this).val() === DEFAULT_LONG_ANSWER_TEXT) { Line too long http://codereview.appspot.com/82042/diff/1001/2003#newcode143 Line 143: widget.find('a.delete img').click(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode159 Line 159: deleted_input.attr({'id': '__deleted__'}).attr({'name': '__deleted__'}); Line too long http://codereview.appspot.com/82042/diff/1001/2003#newcode168 Line 168: widget.find('a.delete_item').click(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode175 Line 175: $('[name=create-option-button]').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode176 Line 176: $(this).click(function () { Need 2 space indentation more from the following line till the end of the anonymous function http://codereview.appspot.com/82042/diff/1001/2003#newcode183 Line 183: .hover(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode189 Line 189: .mousedown(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode192 Line 192: .mouseup(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode197 Line 197: options.find('.AddQuestion').click(function (e) { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode219 Line 219: taking_access_field.change(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode227 Line 227: var CHOOSE_A_PROJECT_FIELD = '<tr class="role-specific"><th><label>Choose Project:</label></th><td> <select disabled=TRUE id="id_survey__NA__selection__project" name="survey__1__selection__see"><option>Survey Taker\'s Projects For This Program</option></select> </td></tr>'; Line too long, you can divide strings with concatenation or (better) with: ["string1","string2","string3"].join(""); http://codereview.appspot.com/82042/diff/1001/2003#newcode228 Line 228: var CHOOSE_A_GRADE_FIELD = '<tr class="role-specific"><th><label>Assign Grade:</label></th><td> <select disabled=TRUE id="id_survey__NA__selection__grade" name="survey__1__selection__see"><option>Pass/Fail</option></select> </td></tr>'; Line too long http://codereview.appspot.com/82042/diff/1001/2003#newcode234 Line 234: case "mentor evaluation": Fix indentation http://codereview.appspot.com/82042/diff/1001/2003#newcode253 Line 253: $('form').bind('submit', function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode266 Line 266: widget.find('.long_answer,input').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode273 Line 273: widget.find('input').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode280 Line 280: widget.find('.long_answer').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode287 Line 287: $('input#id_s_html').val(widget.find('div#survey_options').remove().end().html()); Line too long http://codereview.appspot.com/82042/diff/1001/2003#newcode291 Line 291: survey.find('.sortable').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode295 Line 295: }); As written in the first line.. }(jQuery)); http://codereview.appspot.com/82042/diff/1001/2003#newcode318 Line 318: $(function () { wrap all stuff into: (function ($) { }(jQuery)); like in default-text.js http://codereview.appspot.com/82042/diff/1001/2003#newcode319 Line 319: $(".sortable").each(function (i, domEle) { function () in a new line http://codereview.appspot.com/82042/diff/1001/2003#newcode328 Line 328: $(function () { wrap all stuff into: (function ($) { }(jQuery)); like in default-text.js http://codereview.appspot.com/82042/diff/1001/2003#newcode343 Line 343: $(function () { wrap all stuff into: (function ($) { }(jQuery)); like in default-text.js http://codereview.appspot.com/82042/diff/1001/2003#newcode389 Line 389: '" >' + '</li>') As written in line 227, use ["string1","string2"].join(""), it's faster than concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode406 Line 406: $(function () { wrap all stuff into: (function ($) { }(jQuery)); like in default-text.js http://codereview.appspot.com/82042/diff/1001/2003#newcode418 Line 418: var field_template = $("<tr><th><label>" + del_el + "</label></th><td> </td></tr>"); Line too long, use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode432 Line 432: case "short_answer": Fix indentation inside switch http://codereview.appspot.com/82042/diff/1001/2003#newcode436 Line 436: new_field = "<textarea cols='40' rows='" + MIN_ROWS + "' class='long_answer'/>"; Line too long, use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode439 Line 439: new_field = "<select><option></option>" + default_option + "</select>"; Line too long, use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode442 Line 442: new_field = "<fieldset class='fieldset'><input type='button' value='" + DEFAULT_OPTION_TEXT + "' /></fieldset>"; Line too long, use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode445 Line 445: new_field = "<fieldset class='fieldset'><input type='button' value='" + DEFAULT_OPTION_TEXT + "' /></fieldset>"; Line too long, use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode450 Line 450: var question_for = ('\n <input type="hidden" name="NEW_' + field_name + Use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode455 Line 455: var formatted_name = (SURVEY_PREFIX + new_field_count + type + Line too long, use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode459 Line 459: new_field = $('<fieldset>\n <label for="type_for_' + name + For all this, use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode487 Line 487: Too many newlines http://codereview.appspot.com/82042/diff/1001/2003#newcode499 Line 499: option_html = $('<li id="id-li-' + name + '_' + i + Use .join("") for concatenation http://codereview.appspot.com/82042/diff/1001/2003#newcode516 Line 516: $(new_field).attr({ 'id': 'id_' + formatted_name, 'name': formatted_name }); Line too long, I suggest $(new_field).attr({ 'id': .... }); http://codereview.appspot.com/82042/diff/1001/2003#newcode539 Line 539: }); As written in the first line.. }(jQuery)); http://codereview.appspot.com/82042/diff/1001/2004 File app/soc/content/js/take_survey.js (right): http://codereview.appspot.com/82042/diff/1001/2004#newcode15 Line 15: Add authors /** * @author <a href="mailto:your@mail.box">Name Surname</a> */ http://codereview.appspot.com/82042/diff/1001/2004#newcode17 Line 17: $(function () { wrap all stuff into: (function ($) { }(jQuery)); like in default-text.js http://codereview.appspot.com/82042/diff/1001/2004#newcode18 Line 18: Too many new lines http://codereview.appspot.com/82042/diff/1001/2004#newcode39 Line 39: widget.find('input').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2004#newcode43 Line 43: widget.find('textarea').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2004#newcode52 Line 52: widget.find('textarea').each(function () { function () in a new line http://codereview.appspot.com/82042/diff/1001/2004#newcode55 Line 55: .find('.pick_multi').each(function() { function () in a new line Also missing space between "function" and parens http://codereview.appspot.com/82042/diff/1001/2004#newcode56 Line 56: $(this).find('input').each(function(){ function () in a new line Also missing space between "function" and parens The function content is commented... is still needed? http://codereview.appspot.com/82042/diff/1001/2004#newcode58 Line 58: }); Fix indentation http://codereview.appspot.com/82042/diff/1001/2004#newcode61 Line 61: } Fix indentation http://codereview.appspot.com/82042/diff/1001/2004#newcode62 Line 62: Too many new lines http://codereview.appspot.com/82042/diff/1001/2004#newcode70 Line 70: .change(function(){ .change( in the former line function () in a new line Also missing space between "function" and parens http://codereview.appspot.com/82042/diff/1001/2004#newcode71 Line 71: if ($(this).attr('id') == 'id_project') return; Use === instead of == Wrap statements into ifs in {} (with proper new lines and indentations) even if only one line http://codereview.appspot.com/82042/diff/1001/2004#newcode74 Line 74: $('select#id_project').change(function(){ function () in a new line Also missing space between "function" and parenthesis http://codereview.appspot.com/82042/diff/1001/2004#newcode75 Line 75: if ($('form:first').data('touched') == true) Use === instead of == http://codereview.appspot.com/82042/diff/1001/2004#newcode76 Line 76: { Parenthesis in the former line http://codereview.appspot.com/82042/diff/1001/2004#newcode77 Line 77: var save_check = confirm( Fix indentations http://codereview.appspot.com/82042/diff/1001/2004#newcode80 Line 80: if (!save_check) return false; Wrap statements into ifs in {} (with proper new lines and indentations) even if only one line http://codereview.appspot.com/82042/diff/1001/2004#newcode83 Line 83: if ($(this).val() != 'None') Use !== instead of != Wrap statements into ifs in {} (with proper new lines and indentations) even if only one line http://codereview.appspot.com/82042/diff/1001/2004#newcode84 Line 84: window.location=window.location.href.split('?')[0] + "?project=" + $(this).val(); Line too long Spaces between variable and "=" sign: so window.location = window.location.... http://codereview.appspot.com/82042/diff/1001/2004#newcode93 Line 93: $('input[type=submit]').bind('click', function(e) { 'click' in a new line with indentation function () in another new line... like this: .bind( 'click', function (e) { } ); Also missing space between "function" and parenthesis http://codereview.appspot.com/82042/diff/1001/2004#newcode97 Line 97: $('select#id_project').val() == 'None') Use === instead of == Wrap statements into ifs in {} (with proper new lines and indentations) even if only one line http://codereview.appspot.com/82042/diff/1001/2004#newcode100 Line 100: $('select#id_grade').val() == 'None') Use === instead of == Wrap statements into ifs in {} (with proper new lines and indentations) even if only one line http://codereview.appspot.com/82042/diff/1001/2004#newcode110 Line 110: $('form').bind('submit', function() { function () in a new line Also missing space between "function" and parenthesis http://codereview.appspot.com/82042/diff/1001/2004#newcode111 Line 111: Delete this new line http://codereview.appspot.com/82042/diff/1001/2004#newcode113 Line 113: widget.find('div#survey_options').remove().end().html() Fix indentation http://codereview.appspot.com/82042/diff/1001/2004#newcode116 Line 116: }); As written in the first line.. }(jQuery));
Sign in to reply to this message.
Mario, a *huge*, never-ending stream of thank you very much for this great review! :) I've done extra JSLint runs and opened an issue for the renaming as you suggest. We might also want to introduce version info for these files once they are stable, if the IE cache issue is a concern. Thank you again, and start round 2 whenever you want to :) http://codereview.appspot.com/82042/diff/1001/2001 File app/soc/content/css/soc-090602.css (right): http://codereview.appspot.com/82042/diff/1001/2001#newcode648 Line 648: On 2009/06/23 23:42:36, Mario Ferraro wrote: > From here to line 671 it lacks indentation Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode660 Line 660: /*margin-left:-100px;*/ On 2009/06/23 23:42:36, Mario Ferraro wrote: > Is this still necessary? Don't think so :) Removed. http://codereview.appspot.com/82042/diff/1001/2001#newcode668 Line 668: div #survey_widget th, div #survey_widget td { On 2009/06/23 23:42:36, Mario Ferraro wrote: > Never thought about it, but I think the style you have in lines 687 for multiple > selectors in multiple lines it's a good idea, should be adopted by us.. so go > for it :) But remember to indent! Fixed indentation. http://codereview.appspot.com/82042/diff/1001/2001#newcode680 Line 680: width:320px; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Indentation Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode683 Line 683: div #survey_widget td.formfieldlabel { On 2009/06/23 23:42:36, Mario Ferraro wrote: > It seems there are two spaces between widget and td Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode684 Line 684: font-size:medium; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Indentation Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode695 Line 695: #survey_widget .quant_radio input, #survey_widget .quant_radio label { On 2009/06/23 23:42:36, Mario Ferraro wrote: > See style in line 687 You mean removing the space before the dot? (goes back checking) Fixed and one other above. Ah, one selector per line, done. http://codereview.appspot.com/82042/diff/1001/2001#newcode720 Line 720: color:#666666; On 2009/06/23 23:42:36, Mario Ferraro wrote: > indentation Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode730 Line 730: .sortable_li {border: solid 1px !important; padding:5px !important;} On 2009/06/23 23:42:36, Mario Ferraro wrote: > I suggest to put it in multiple lines Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode733 Line 733: .sortable_li, div #survey_widget fieldset{ On 2009/06/23 23:42:36, Mario Ferraro wrote: > See style in line 687 Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode735 Line 735: -webkit-border-radius: 5px; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Indentation Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode738 Line 738: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Fix new lines and indentation from here to line 744 Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode747 Line 747: .ui-button {outline: 0; margin:0; padding: .4em 1em .5em; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Change the whole class and put each statement line per line Done line per line, can you explain the change the whole class part? :) http://codereview.appspot.com/82042/diff/1001/2001#newcode751 Line 751: .ui-dialog .ui-state-highlight, .ui-dialog .ui-state-error { padding: .3em; } On 2009/06/23 23:42:36, Mario Ferraro wrote: > See style in line 687 Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode765 Line 765: padding:0 20px; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Indentation Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode783 Line 783: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Delete this line :) Done. http://codereview.appspot.com/82042/diff/1001/2001#newcode802 Line 802: margin:-1px 0; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Indentation Done. http://codereview.appspot.com/82042/diff/1001/2002 File app/soc/content/js/default-text.js (right): http://codereview.appspot.com/82042/diff/1001/2002#newcode1 Line 1: (function ($) { On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line for better readability Done. http://codereview.appspot.com/82042/diff/1001/2002#newcode1 Line 1: (function ($) { On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line for better readability Done. http://codereview.appspot.com/82042/diff/1001/2002#newcode1 Line 1: (function ($) { On 2009/06/23 23:42:36, Mario Ferraro wrote: > I suggest to put all your js files to have a prefix of "surveys-", to better > separate your code, waiting for the whole "package-like" refactoring before or > then :) > Also I don't see any "_" to separate words in Javascript files name in the > current scripts, so ... well is up to you to keep the style or not :) > > Add copyright notices to all the files and also authors: > /** > * @author <a href="mailto:your@mail.box">Name Surname</a> > */ Done, opened an issue for the renaming at http://github.com/jamslevy/Melange/issues/#issue/41. http://codereview.appspot.com/82042/diff/1001/2002#newcode2 Line 2: var map = []; On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2002#newcode3 Line 3: $.preserveDefaultText = { On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2002#newcode14 Line 14: }, On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2002#newcode27 Line 27: } On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2002#newcode31 Line 31: var defaultColor = input.css("color"); On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2002#newcode37 Line 37: }; On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2002#newcode43 Line 43: } On 2009/06/23 23:42:36, Mario Ferraro wrote: > add a semicolon Done. JSLint doesn't like it. http://codereview.appspot.com/82042/diff/1001/2002#newcode53 Line 53: } On 2009/06/23 23:42:36, Mario Ferraro wrote: > add a semicolon Done. http://codereview.appspot.com/82042/diff/1001/2003 File app/soc/content/js/edit_survey.js (right): http://codereview.appspot.com/82042/diff/1001/2003#newcode15 Line 15: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Add authors > > /** > * @author <a href="mailto:your@mail.box">Name Surname</a> > */ Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode16 Line 16: var DEFAULT_LONG_ANSWER_TEXT = 'Write a Custom Prompt For This Question...'; On 2009/06/23 23:42:36, Mario Ferraro wrote: > These variables probably will affect global scope, should be inside the scope > where they're needed Fixed 5 that were local, should the other 3 be defined locally twice? http://codereview.appspot.com/82042/diff/1001/2003#newcode29 Line 29: $(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > wrap all stuff into: > (function ($) { > }(jQuery)); > > like in default-text.js Done. I suppose I can't get away with not re-indenting the whole file, can I? :) http://codereview.appspot.com/82042/diff/1001/2003#newcode34 Line 34: var widget = $('div#survey_widget'); On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode36 Line 36: 'float': 'left', On 2009/06/23 23:42:36, Mario Ferraro wrote: > 4 space indentation, not 6 Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode38 Line 38: }); On 2009/06/23 23:42:36, Mario Ferraro wrote: > 2 space indentation, not 4 Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode47 Line 47: } On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode55 Line 55: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Fix the comment Done. I mean, done moving the // Bind submit comment. But I've just noticed some /**/ comments are indented, others not. JSLint doesn't give me a clue here... I guess we should keep the comment in the same indent level as code? Indented comment. http://codereview.appspot.com/82042/diff/1001/2003#newcode56 Line 56: var survey_html = $('form').find("#id_survey_html").attr('value'); On 2009/06/23 23:42:36, Mario Ferraro wrote: > append a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode78 Line 78: widget.find('.short_answer').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > When anonymous functions are injected as parameters, I suggest (for readability) > to have a new line for "function () {" and fix indentations consequently. > Something like > > widget.find().each( > function () { > blah blah > } > ); > > I'll be writing this when encountered, then we'll talk about it eventually :) Thanks, I got confused in this case a few times and your suggestion makes it tidier, fixing them all :) http://codereview.appspot.com/82042/diff/1001/2003#newcode83 Line 83: widget.find('.long_answer').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode95 Line 95: $('form input, form button, form select').keypress(function (e) { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode105 Line 105: $('a.fetch_answers').click(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode109 Line 109: var query = '?read_only=true&user_results=' + user; On 2009/06/23 23:42:36, Mario Ferraro wrote: > be careful not introducing cache issues. If it happens, I suggest to append > &_"+(new Date().getTime()) Added TODO. http://codereview.appspot.com/82042/diff/1001/2003#newcode124 Line 124: survey.bind('init', function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode128 Line 128: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Comment lines are too long, I suggest to introduce "TODO:" or "FIXME:" for this > kind of comments Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode129 Line 129: widget.find('input').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode130 Line 130: if (($(this).val().length < 1 || $(this).val() === DEFAULT_SHORT_ANSWER_TEXT) && On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode136 Line 136: widget.find('.long_answer').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode137 Line 137: if ($(this).val().length < 1 || $(this).val() === DEFAULT_LONG_ANSWER_TEXT) { On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode143 Line 143: widget.find('a.delete img').click(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode159 Line 159: deleted_input.attr({'id': '__deleted__'}).attr({'name': '__deleted__'}); On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode168 Line 168: widget.find('a.delete_item').click(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done (moved down about 60 lines in new rev). http://codereview.appspot.com/82042/diff/1001/2003#newcode175 Line 175: $('[name=create-option-button]').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode176 Line 176: $(this).click(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > Need 2 space indentation more from the following line till the end of the > anonymous function Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode183 Line 183: .hover(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode189 Line 189: .mousedown(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode192 Line 192: .mouseup(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode197 Line 197: options.find('.AddQuestion').click(function (e) { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode219 Line 219: taking_access_field.change(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode227 Line 227: var CHOOSE_A_PROJECT_FIELD = '<tr class="role-specific"><th><label>Choose Project:</label></th><td> <select disabled=TRUE id="id_survey__NA__selection__project" name="survey__1__selection__see"><option>Survey Taker\'s Projects For This Program</option></select> </td></tr>'; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long, you can divide strings with concatenation or (better) with: > ["string1","string2","string3"].join(""); Done. (Is this 'indent array one level' trick allowed? Saves us from lots of narrow lines...) http://codereview.appspot.com/82042/diff/1001/2003#newcode228 Line 228: var CHOOSE_A_GRADE_FIELD = '<tr class="role-specific"><th><label>Assign Grade:</label></th><td> <select disabled=TRUE id="id_survey__NA__selection__grade" name="survey__1__selection__see"><option>Pass/Fail</option></select> </td></tr>'; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode234 Line 234: case "mentor evaluation": On 2009/06/23 23:42:36, Mario Ferraro wrote: > Fix indentation If I increase it after {, JSLint complains. Should I increase anyway? http://codereview.appspot.com/82042/diff/1001/2003#newcode253 Line 253: $('form').bind('submit', function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode266 Line 266: widget.find('.long_answer,input').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode273 Line 273: widget.find('input').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode280 Line 280: widget.find('.long_answer').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode287 Line 287: $('input#id_s_html').val(widget.find('div#survey_options').remove().end().html()); On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode291 Line 291: survey.find('.sortable').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode295 Line 295: }); On 2009/06/23 23:42:36, Mario Ferraro wrote: > As written in the first line.. > }(jQuery)); Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode318 Line 318: $(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > wrap all stuff into: > (function ($) { > }(jQuery)); > > like in default-text.js Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode319 Line 319: $(".sortable").each(function (i, domEle) { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode328 Line 328: $(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > wrap all stuff into: > (function ($) { > }(jQuery)); > > like in default-text.js Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode343 Line 343: $(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > wrap all stuff into: > (function ($) { > }(jQuery)); > > like in default-text.js Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode389 Line 389: '" >' + '</li>') On 2009/06/23 23:42:36, Mario Ferraro wrote: > As written in line 227, use > ["string1","string2"].join(""), it's faster than concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode406 Line 406: $(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > wrap all stuff into: > (function ($) { > }(jQuery)); > > like in default-text.js Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode418 Line 418: var field_template = $("<tr><th><label>" + del_el + "</label></th><td> </td></tr>"); On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long, use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode432 Line 432: case "short_answer": On 2009/06/23 23:42:36, Mario Ferraro wrote: > Fix indentation inside switch Same as before: should I do this against JSLint's gut feeling? :) http://codereview.appspot.com/82042/diff/1001/2003#newcode436 Line 436: new_field = "<textarea cols='40' rows='" + MIN_ROWS + "' class='long_answer'/>"; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long, use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode439 Line 439: new_field = "<select><option></option>" + default_option + "</select>"; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long, use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode442 Line 442: new_field = "<fieldset class='fieldset'><input type='button' value='" + DEFAULT_OPTION_TEXT + "' /></fieldset>"; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long, use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode445 Line 445: new_field = "<fieldset class='fieldset'><input type='button' value='" + DEFAULT_OPTION_TEXT + "' /></fieldset>"; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long, use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode450 Line 450: var question_for = ('\n <input type="hidden" name="NEW_' + field_name + On 2009/06/23 23:42:36, Mario Ferraro wrote: > Use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode455 Line 455: var formatted_name = (SURVEY_PREFIX + new_field_count + type + On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long, use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode459 Line 459: new_field = $('<fieldset>\n <label for="type_for_' + name + On 2009/06/23 23:42:36, Mario Ferraro wrote: > For all this, use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode487 Line 487: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Too many newlines Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode499 Line 499: option_html = $('<li id="id-li-' + name + '_' + i + On 2009/06/23 23:42:36, Mario Ferraro wrote: > Use .join("") for concatenation Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode516 Line 516: $(new_field).attr({ 'id': 'id_' + formatted_name, 'name': formatted_name }); On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long, I suggest > $(new_field).attr({ > 'id': .... > }); Done. http://codereview.appspot.com/82042/diff/1001/2003#newcode539 Line 539: }); On 2009/06/23 23:42:36, Mario Ferraro wrote: > As written in the first line.. > }(jQuery)); Done. http://codereview.appspot.com/82042/diff/1001/2004 File app/soc/content/js/take_survey.js (right): http://codereview.appspot.com/82042/diff/1001/2004#newcode15 Line 15: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Add authors > > /** > * @author <a href="mailto:your@mail.box">Name Surname</a> > */ Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode17 Line 17: $(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > wrap all stuff into: > (function ($) { > }(jQuery)); > > like in default-text.js Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode18 Line 18: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Too many new lines Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode39 Line 39: widget.find('input').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode43 Line 43: widget.find('textarea').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode52 Line 52: widget.find('textarea').each(function () { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode55 Line 55: .find('.pick_multi').each(function() { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line > Also missing space between "function" and parens Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode56 Line 56: $(this).find('input').each(function(){ On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line > Also missing space between "function" and parens > The function content is commented... is still needed? Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode58 Line 58: }); On 2009/06/23 23:42:36, Mario Ferraro wrote: > Fix indentation Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode61 Line 61: } On 2009/06/23 23:42:36, Mario Ferraro wrote: > Fix indentation Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode62 Line 62: On 2009/06/23 23:42:36, Mario Ferraro wrote: > Too many new lines Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode70 Line 70: .change(function(){ On 2009/06/23 23:42:36, Mario Ferraro wrote: > .change( in the former line > function () in a new line > Also missing space between "function" and parens Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode71 Line 71: if ($(this).attr('id') == 'id_project') return; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Use === instead of == > Wrap statements into ifs in {} (with proper new lines and indentations) even if > only one line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode74 Line 74: $('select#id_project').change(function(){ On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line > Also missing space between "function" and parenthesis Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode75 Line 75: if ($('form:first').data('touched') == true) On 2009/06/23 23:42:36, Mario Ferraro wrote: > Use === instead of == Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode76 Line 76: { On 2009/06/23 23:42:36, Mario Ferraro wrote: > Parenthesis in the former line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode77 Line 77: var save_check = confirm( On 2009/06/23 23:42:36, Mario Ferraro wrote: > Fix indentations Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode80 Line 80: if (!save_check) return false; On 2009/06/23 23:42:36, Mario Ferraro wrote: > Wrap statements into ifs in {} (with proper new lines and indentations) even if > only one line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode83 Line 83: if ($(this).val() != 'None') On 2009/06/23 23:42:36, Mario Ferraro wrote: > Use !== instead of != > Wrap statements into ifs in {} (with proper new lines and indentations) even if > only one line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode84 Line 84: window.location=window.location.href.split('?')[0] + "?project=" + $(this).val(); On 2009/06/23 23:42:36, Mario Ferraro wrote: > Line too long > Spaces between variable and "=" sign: so > window.location = window.location.... Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode93 Line 93: $('input[type=submit]').bind('click', function(e) { On 2009/06/23 23:42:36, Mario Ferraro wrote: > 'click' in a new line with indentation > function () in another new line... like this: > > .bind( > 'click', > function (e) { > } > ); > > Also missing space between "function" and parenthesis Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode97 Line 97: $('select#id_project').val() == 'None') On 2009/06/23 23:42:36, Mario Ferraro wrote: > Use === instead of == > Wrap statements into ifs in {} (with proper new lines and indentations) even if > only one line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode100 Line 100: $('select#id_grade').val() == 'None') On 2009/06/23 23:42:36, Mario Ferraro wrote: > Use === instead of == > Wrap statements into ifs in {} (with proper new lines and indentations) even if > only one line Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode110 Line 110: $('form').bind('submit', function() { On 2009/06/23 23:42:36, Mario Ferraro wrote: > function () in a new line > Also missing space between "function" and parenthesis Should we make all ".method('name', function () {" follow the style of ".bind( 'click', function (e) { " as above? http://codereview.appspot.com/82042/diff/1001/2004#newcode113 Line 113: widget.find('div#survey_options').remove().end().html() On 2009/06/23 23:42:36, Mario Ferraro wrote: > Fix indentation Done. http://codereview.appspot.com/82042/diff/1001/2004#newcode116 Line 116: }); On 2009/06/23 23:42:36, Mario Ferraro wrote: > As written in the first line.. > }(jQuery)); Done.
Sign in to reply to this message.
Great work ^__^ Just few things to correct, one thing I was wrong (fixed our wiki styleguide ^__^) and so to rollback and two style proposals we might want to talk about :) Cheers, Mario Ferraro http://codereview.appspot.com/82042/diff/1008/2010 File app/soc/content/css/soc-090602.css (right): http://codereview.appspot.com/82042/diff/1008/2010#newcode1 Line 1: /* For the whole file: I was thinking it could be good to agree on using a space after ":" for every statement like this: .classwhatever { width: 50px; } instead of .classwhatever { width:50px; } I see you use both, perhaps we want to do it only in the former way. Just a suggestion :) http://codereview.appspot.com/82042/diff/1008/2010#newcode661 Line 661: Ehm :) Now it's void... it's still needed to be filled afterwards? http://codereview.appspot.com/82042/diff/1008/2010#newcode669 Line 669: div #survey_widget th, div #survey_widget td { Sorry I didn't understand if you liked the idea or if you missed the change :) If it's so, just put every selector in a line like this: div #survey_widget th, div #survey_widget td { } http://codereview.appspot.com/82042/diff/1008/2010#newcode781 Line 781: .ui-button { Ok I was meaning exactly that ^__^ http://codereview.appspot.com/82042/diff/1008/2011 File app/soc/content/js/default-text.js (right): http://codereview.appspot.com/82042/diff/1008/2011#newcode72 Line 72: }; Uhm, you're right... I was very wrong here, it's unnecessary and packers would like it anyway... it's when you're storing anonymous functions inside variables that is necessary. Changed the style guide in our wiki accordingly... and well... please forgive me if I ask you to delete it :D :D http://codereview.appspot.com/82042/diff/1008/2011#newcode82 Line 82: }; Delete it again without hating me too much :P http://codereview.appspot.com/82042/diff/1008/2012 File app/soc/content/js/edit_survey.js (right): http://codereview.appspot.com/82042/diff/1008/2012#newcode1 Line 1: /* Copyright 2009 the Melange authors. Just a general style question to you for using jQuery (we're enhancing style guide here, cool ^__^). I find code in lines 199 (with every chained function in a new line) more readable than normal chains like do().something().now(). If you find it also more readable we might want to change it everywhere (if we're in a hurry to push, just do it afterwards). http://codereview.appspot.com/82042/diff/1008/2012#newcode26 Line 26: var SURVEY_PREFIX = 'survey__'; I think you can declare them twice or wrap all the script inside another (function() { })(); Don't know if it would work good with this script, if not try also to wrap it into: (function() { }); or var survey = function () { } it depends on what you have in HTML and the overall functioning of the script. Just make some attempts ^__^ http://codereview.appspot.com/82042/diff/1008/2012#newcode60 Line 60: Yes you're completely right, every comment should be indented at the same level of the block it comments :) http://codereview.appspot.com/82042/diff/1008/2012#newcode92 Line 92: widget.find('.long_answer,input').each(function () { Anonymous function in a new line http://codereview.appspot.com/82042/diff/1008/2012#newcode281 Line 281: Perfect :) The trick is allowed also with jsLint. What that jsLint doesn't like is doing something like this: var foo = [ '<div>', '<table>', '</table>', '</div>' ] which, however, is something more readable. We might want to see what can be done in jsLint to avoid checking of arrays indentation, or just try something else like browser side templates (I'm exploring that solution these days) http://codereview.appspot.com/82042/diff/1008/2012#newcode293 Line 293: switch (role_type) { Hmm I think it's ok also with this kind of indentation http://codereview.appspot.com/82042/diff/1008/2012#newcode360 Line 360: .end().html()); Ok, this is an example of what I meant in the first line: doing something like the following perhaps improves readability. Just a proposal. $('input#id_s_html') .val( widget .find( 'div#survey_options' ) .remove() .end() .html() ); http://codereview.appspot.com/82042/diff/1008/2012#newcode536 Line 536: switch (button_id) { Just leave this indentation it's fine I think :) http://codereview.appspot.com/82042/diff/1008/2013 File app/soc/content/js/take_survey.js (right): http://codereview.appspot.com/82042/diff/1008/2013#newcode81 Line 81: // remember if form has been touched Fix comment indentation
Sign in to reply to this message.
Thanks again for reviewing, Mario. Regarding the function per line style, we have to avoid making code harder to read by stretching it too high. I'm afraid three lines per-call (assuming one argument) tends to make chainy code too high, so we could make it less chainy, use a rule that lowers the average lines per-call, etc. (if this is a concern) :) http://codereview.appspot.com/82042/diff/1008/2010 File app/soc/content/css/soc-090602.css (right): http://codereview.appspot.com/82042/diff/1008/2010#newcode1 Line 1: /* On 2009/06/26 12:02:10, Mario Ferraro wrote: > For the whole file: I was thinking it could be good to agree on using a space > after ":" for every statement like this: > > .classwhatever { > width: 50px; > } > > instead of > > .classwhatever { > width:50px; > } > > I see you use both, perhaps we want to do it only in the former way. Just a > suggestion :) I actually wanted to use a space, but missed some places, thank! http://codereview.appspot.com/82042/diff/1008/2010#newcode661 Line 661: On 2009/06/26 12:02:10, Mario Ferraro wrote: > Ehm :) Now it's void... it's still needed to be filled afterwards? Not sure, but as we have to go through CSS and HTML for cross-browser checking, adding a TODO to remove if not needed :) http://codereview.appspot.com/82042/diff/1008/2010#newcode669 Line 669: div #survey_widget th, div #survey_widget td { On 2009/06/26 12:02:10, Mario Ferraro wrote: > Sorry I didn't understand if you liked the idea or if you missed the change :) > If it's so, just put every selector in a line like this: > > div #survey_widget th, > div #survey_widget td { > } Missed this one :) Done,thanks! http://codereview.appspot.com/82042/diff/1008/2010#newcode781 Line 781: .ui-button { On 2009/06/26 12:02:10, Mario Ferraro wrote: > Ok I was meaning exactly that ^__^ Cool :) http://codereview.appspot.com/82042/diff/1008/2011 File app/soc/content/js/default-text.js (right): http://codereview.appspot.com/82042/diff/1008/2011#newcode72 Line 72: }; On 2009/06/26 12:02:10, Mario Ferraro wrote: > Uhm, you're right... I was very wrong here, it's unnecessary and packers would > like it anyway... it's when you're storing anonymous functions inside variables > that is necessary. Changed the style guide in our wiki accordingly... and > well... please forgive me if I ask you to delete it :D :D No problem, done :) http://codereview.appspot.com/82042/diff/1008/2011#newcode82 Line 82: }; On 2009/06/26 12:02:10, Mario Ferraro wrote: > Delete it again without hating me too much :P Done. http://codereview.appspot.com/82042/diff/1008/2012 File app/soc/content/js/edit_survey.js (right): http://codereview.appspot.com/82042/diff/1008/2012#newcode1 Line 1: /* Copyright 2009 the Melange authors. On 2009/06/26 12:02:10, Mario Ferraro wrote: > Just a general style question to you for using jQuery (we're enhancing style > guide here, cool ^__^). > I find code in lines 199 (with every chained function in a new line) more > readable than normal chains like do().something().now(). > If you find it also more readable we might want to change it everywhere (if > we're in a hurry to push, just do it afterwards). I like the idea, but I think we should discuss it a bit more. I don't believe I'll have time change the style in all the code before it's July, as we still have features, testing and fixes to add/do/make. Would we indent on arguments? Would field_template.find('label').attr('for', 'NEW_' + name).append(question_content).end().find('td').append(new_field); become field_template.find('label') .attr('for', 'NEW_' + name) .append(question_content) .end() .find('td') .append(new_field); or field_template.find( 'label' ) .attr( 'for', 'NEW_' + name ) .append( question_content ) .end() .find( 'td' ) .append( new_field ); I'd like to use a less chainy style (maybe unidiomatic JS or jQuery), and we might want to have a rule of thumb for when to break lines (e.g. after we change the target object): field_template.find('label') .attr('for', 'NEW_' + name); field_template.append(question_content).end(); // or is the append to the label? :) field_template.find('td') .append(new_field); http://codereview.appspot.com/82042/diff/1008/2012#newcode26 Line 26: var SURVEY_PREFIX = 'survey__'; On 2009/06/26 12:02:10, Mario Ferraro wrote: > I think you can declare them twice or wrap all the script inside another > > (function() { > })(); > > Don't know if it would work good with this script, if not try also to wrap it > into: > > (function() { > }); > > or > > var survey = function () { > } > > it depends on what you have in HTML and the overall functioning of the script. > Just make some attempts ^__^ I've redeclared them, will test wrapping everything later :) http://codereview.appspot.com/82042/diff/1008/2012#newcode92 Line 92: widget.find('.long_answer,input').each(function () { On 2009/06/26 12:02:10, Mario Ferraro wrote: > Anonymous function in a new line Missed this one, sorry. Done http://codereview.appspot.com/82042/diff/1008/2012#newcode281 Line 281: On 2009/06/26 12:02:10, Mario Ferraro wrote: > Perfect :) The trick is allowed also with jsLint. What that jsLint doesn't like > is doing something like this: > > var foo = [ > '<div>', > '<table>', > '</table>', > '</div>' > ] > > which, however, is something more readable. We might want to see what can be > done in jsLint to avoid checking of arrays indentation, or just try something > else like browser side templates (I'm exploring that solution these days) > How about var foo = [ '<div>', ' <table>', ' </table>', '</div>' ] ? http://codereview.appspot.com/82042/diff/1008/2012#newcode293 Line 293: switch (role_type) { On 2009/06/26 12:02:10, Mario Ferraro wrote: > Hmm I think it's ok also with this kind of indentation Done. http://codereview.appspot.com/82042/diff/1008/2012#newcode360 Line 360: .end().html()); On 2009/06/26 12:02:10, Mario Ferraro wrote: > Ok, this is an example of what I meant in the first line: doing something like > the following perhaps improves readability. Just a proposal. > > $('input#id_s_html') > .val( > widget > .find( > 'div#survey_options' > ) > .remove() > .end() > .html() > ); > Clear win here, done :) http://codereview.appspot.com/82042/diff/1008/2012#newcode536 Line 536: switch (button_id) { On 2009/06/26 12:02:10, Mario Ferraro wrote: > Just leave this indentation it's fine I think :) Done. http://codereview.appspot.com/82042/diff/1008/2013 File app/soc/content/js/take_survey.js (right): http://codereview.appspot.com/82042/diff/1008/2013#newcode81 Line 81: // remember if form has been touched On 2009/06/26 12:02:10, Mario Ferraro wrote: > Fix comment indentation Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
