launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Please address the issues pointed as inline comments and we can have another review-round. 

Inline comments:

> --- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-15 04:18:54 +0000
> +++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-19 17:53:21 +0000
> @@ -475,7 +475,7 @@
>       *
>       * @method _connectScrollers
>       */
> -    _connectScrollers: function() {
> +    _connectScrollers: function(rc_scroller) {
>          var self = this;
>          var rc = Y.lp.code.branchmergeproposal.reviewcomment;
>          Y.all('td[data-previewdiff-id]').each( function(td) {
> @@ -499,6 +499,17 @@
>                  self._showPreviewDiff(previewdiff_id);
>              });
>          });
> +        
> +        rc.link_scroller(rc_scroller, '#add-comment', function() {
> +            var drafts = Y.all('.draft');
> +
> +            var has_drafts = drafts.size() > 0;
> +            Y.one('#add-comment-form input[type=checkbox]')
> +                .set('checked', has_drafts);
> +            Y.one('#add-comment-form input[type=submit]')
> +                .set('disabled', !has_drafts)
> +            Y.one('#add-comment-form input[type=submit]').focus();
> +        });

You only need to focus on the submit button, all the rest of the updating/setup is already done by PublishDraft() widget.
Also, you should look for a solution that does not focus on the button if there are no drafts, maybe focus on the textbox in that case.

>      },
>  
>      /**
> @@ -608,6 +619,12 @@
>                          navigator.append(option);
>                      });
>                      self.set('navigator', navigator);
> +                    var publish_link = Y.Node.create('<a id="publish"/>')
> +                        .addClass('review-comment-scroller')
> +                        .set('href', '')
> +                        .set('text', 'Publish inline comments');
> +                    Y.Node.one('#review-diff').append(publish_link);
> +                    self._connectScrollers(publish_link);

The way things are at moment, you are more than welcome to move the tag creation and placement to the _connectScroller() method. It is already creating the other scroller in JS, no need to pass the element in for now.
Later on we can encapsulate all scrollers in a new widget if it turns out to be beneficial.

>                  },
>                  failure: function(ignore, response, args) {
>                      Y.log('DiffNav: ' + preview_diffs_uri +
> 
> === modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-15 03:47:40 +0000
> +++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-19 17:53:21 +0000
> @@ -524,6 +524,33 @@
>              Y.Assert.areEqual(202, called_pd_id);
>          },
>  
> +        test_diff_nav_publish_scroller: function() {
> +            // The Diff Navigator publish comment *scroller* is connected
> +            // upon widget rendering and when clicked triggers the browser
> +            // to scroll to the comment textbox.
> +            this.diffnav.render();
> +            this.reply_previewdiffs();
> +            this.reply_diffcontent();
> +            var mockio = this.diffnav.get('lp_client').io_provider;
> +            Y.Assert.areEqual(2, mockio.requests.length);
> +            // We need to re-instrument the scroller in other to
> +            // instantly fire the 'end' event (which runs the code
> +            // that matters to us).
> +            scroller = Y.one('#publish');
> +            console.log(scroller)
> +            Y.Assert.areEqual('Publish inline comments',

this line should be dropped before commit. If logging become necessary in your changes (like we do for failures), user Y.log().

> +                scroller.get('text'));
> +            scroller.on('click', function() {
> +                var rc = Y.lp.code.branchmergeproposal.reviewcomment;
> +                rc.window_scroll_anim.fire('end');
> +            });
> +            // Click the scroller results in returning to the comment
> +            // textfield.
> +            var active = document.getElementById(document.activeElement);
> +            Y.Assert.areEqual(Y.one('field.actions.add'), active);
> +

This CSS selector does not work, i.e. Y.one('field.actions.add') is null. So I presume 'active' is null as well.
Why don't you simply check for areEqual('field.actions.add', document.activeElement.id) ?

> +        },
> +        
>          test_diff_nav_failed_diff_content: function() {
>              // An error message is presented when the Diff Navigator
>              // fails to fetch the selected diff contents.
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/publish-ic-link/+merge/220103
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjohnston/launchpad/publish-ic-link into lp:launchpad.