Code review comment for lp:~yellow/launchpad/accordion-client-1

Revision 12662 has the below-mentioned changes.

> [5]
>
> 479 +namespace.list_contains = list_contains;
>
> This seems to be a common pattern. Why not just make the function part
> of the namespace in the first place:
>
> namespace.list_contains = function(list, target) {
> ///...
> };
>
> (This is a common pattern elsewhere in the LP JS, though I don't know if
> it's considered a standard). I'll not bother highlighting the other
> cases; they're easy enough to find.

This was done to decrease the identifier length. Otherwise instead of
just referencing "list_contains" in the body of the module, we'd have to
use "namespace.list_contains" throughout. The only reason the function
is needed outside of the namespace is for testing so the one-liner to
expose it to tests seems a reasonable trade-off.

> [10]
>
> 633 +function edit_subscription_handler(context, form_data) {
>
> This function needs some documentation commentary.

Done.

>
> [11]
>
> 649 +function create_overlay(content_box_id, overlay_id, submit_button,
> 650 + submit_callback) {

Done.

> [12]
>
> 691 + alert('"Subscribe to bug mail" link not found.');
>
> We shouldn't be using the alert() here any more. Y.fail would seem more
> appropriate.

Fixed.

> [13]
>
> 698 + e.preventDefault();
>
> There's a been a bug about the use of e.preventDefault() and
> e.stopPropagation() not working properly with some browsers (probably IE
> but ISTR some WebKit browsers had problems too). For safety we should
> use e.halt(), since in all the cases where one of {preventDefault,
> stopPropagation} fails, the other works fine, and e.halt() calls both.
> It's probably worth replacing all instances of preventDefault() in this
> JS with halt().

Fixed.

> [14]
>
> 708 +function clear_overlay(content_node) {
>
> This needs documentation.

Done.

> [15]
>
> 902 + * @param node The node to collapse.
> 903 + */
> 904 +function collapse_node(node, user_cfg) {
>
> user_cfg needs to be documented here, too.

Gary and I have been following the (self-imposed) guideline that only
functions meant for "public" consumption require the javadoc-like
documentation so I fixed the asymmetry in parameter documentation by
removing the docs for the other parameter.

> [16]
>
> 939 +/**
> 940 + * Expand the node and set its arrow to 'collapsed'
> 941 + * @param node The node to collapse.
> 942 + */
> 943 +function expand_node(node, user_cfg) {
>
> Same as [15].

Same fix as [15].

> [17]
>
> 977 +/**
> 978 + * Construct the overlay and populate it with the add/edit form.
> 979 + *
> 980 + * @method setup_overlay
> 981 + * @param {String} content_box_id Id of the element on the page where
> 982 + * the overlay is anchored.
> 983 + * @return {String} overlay_id Id of the constructed overlay element.
> 984 + */
> 985 +function setup_overlay(content_box_id, hide_recipient_picker) {
>
> There's no overlay_id parameter, so you can drop the documentation for
> it.

Same fix as [15].

> [18]
>
> 1242 + // if we want to allow editing of the recipient, do this
> instead
> 1243 + // of the if/else block below.
> 1244 + // set_recipient(
> 1245 + // content_node, subscriber_is_team, team_link);
>
> Can this be dropped from the code or are we going to be using it later?

Dropped.

> [19]
>
> These all need documentation:
>
> 1182 +function get_input_by_value(node, value) {
> 1189 +function set_checkboxes(node, all, checked) {
> 1200 +function set_options(node, name, value) {
> 1207 +function set_radio_buttons(node, all, value) {
> 1211 +function set_recipient(node, is_team, team_link) {
> 1220 +function make_edit_handler(subscription, filter_info, filter_id,
> 1221 + config, context) {
> 1309 +function make_delete_handler(filter, filter_id, subscriber_id) {
> 1337 +function wire_up_edit_links(config, context) {
> 1337 +function wire_up_edit_links(config, context) {
> 1436 +function render_filter_name(filter_info, filter) {
> 1450 +function render_filter_description(filter) {
> 1498 +function validate_config(config) {
>
> As do:
>
> 1754 + test_logged_in_user: function() {
> 1761 + test_list_contains: function() {
> 1773 + test_make_selector_controls: function() {
> 1838 + test_user_recipient: function() {
> 1847 + test_team_recipient: function() {
> 1989 + test_importances_select_all_none: function() {
> 2003 + test_statuses_select_all_none: function() {
> 2065 + test_overlay_error_handling_adding: function() {
> 2083 + test_overlay_error_handling_patching: function() {
>
> The test_extract_* tests in the extract_form_data test case could do
> with some documentation too, though I realise there'd be a lot of
> boilerplate there. Maybe one comment for the test case would suffice.

All of these are now documented.

> [20]
>
> 1918 + // When initialized the <div> elements for the filter
> 1919 + // wrapper and the accordion wrapper should be collapsed.
> 1920 + // Since the collapsing is done via animation, we must
> 1921 + // wait a bit for it to happen.
>
> In the comments here you talk about waiting for something to happen, but
> I don't see any calls to this.wait() in this test. Can this statement be
> removed?

I removed that comment. It was outdated.