Docking the Comment Dialog

Review Request #10213 — Created Oct. 10, 2018 and updated

Information

Review Board
master
7b001f4...

Reviewers

Comment Dialog: Docking

When reviews have the tendency to get longer the Comment Dialog
becomes harder to keep track of. To fix this,the comment
dialog has been docked to the page. This means when users open
up a comment, the dialog will stay on the page with the user
during the review process.

Users can drag around the dialog with ease with the use of
jQuery UI and standard jQuery mouse events. Functions have been
implemented to ensure that the Comment Dialog remains in the content
area. This is to prevent users from accidentally moving the dialog
off-screen.

Ran and passed all of the JavaScript Unit Tests.

Added a new test to check whether the position was being set
correctly on mousedown and mouseup.

Description From Last Updated

Col: 13 'newTop' is defined but never used.

reviewbotreviewbot

Col: 53 Missing semicolon.

reviewbotreviewbot

Col: 18 'getTop' is defined but never used.

reviewbotreviewbot

Col: 34 Missing semicolon.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

Most of these could be declared const.

alextechccalextechcc

Col: 18 'getTop' is defined but never used.

reviewbotreviewbot

Col: 34 Missing semicolon.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

Col: 18 'getTop' is defined but never used.

reviewbotreviewbot

Col: 34 Missing semicolon.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

Col: 71 Missing semicolon.

reviewbotreviewbot

Col: 41 Missing semicolon.

reviewbotreviewbot

Col: 15 'isAbove' was used before it was defined.

reviewbotreviewbot

Col: 10 Missing semicolon.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

trailing whitespace.

brenniebrennie

This can just be .css('position', 'fixed'). No need to construct an object. If you want to keep it as an …

brenniebrennie

Trailing whitespace.

brenniebrennie

Only the comments before a function should be /**. Regular multiline comments can use /*. Same below.

brenniebrennie

Trailing whitespace. Missing space after period.

brenniebrennie

This can be a single computation. const top = commentBox.offset().top - 8;

brenniebrennie

What is 8 here? where does this come from?

brenniebrennie

You can replace this with: commentBox.css({ 'position': 'absolute', 'top': `${top}px`, });

brenniebrennie

This can be a single computation. Always space around binary operators.

brenniebrennie

The parentheses are redundant.

brenniebrennie

You can replace this with: commentBox.css({ 'position': 'fixed', 'top': `${top}px`, });

brenniebrennie

Doc comments should be of the form: ``` /* * Single line summary (in imperitive mood). * * Multi-line display. …

brenniebrennie

Trailing whitespace. Dialog -> dialog Checks -> Check.

brenniebrennie

We generally use function statements in this case, e.g. function isAbove() { // ... } Same with isBelow below.

brenniebrennie

This should be named something like isTopPosition because it is a boolean. A variable named topPosition would be, e.g., the …

brenniebrennie

Where does 8 come from?

brenniebrennie

Blank line between these.

brenniebrennie

Replace with: return topPosition || offsetTop === 0;

brenniebrennie

Doc comments should be of the form: ``` /* * Single line summary (in imperitive mood). * * Multi-line display. …

brenniebrennie

Replace with: return commentBox.offset().top > bottomOfScreen;

brenniebrennie

Trailing whitespace.

brenniebrennie

stop: function(event, ui) {

brenniebrennie

You can replace this with: ui.helper.css({ 'position': 'fixed', 'top': `${ui.helper.scrollTop()}px`, });

brenniebrennie

} else if (isBelow()) {

brenniebrennie

You can replace this with: ui.helper.css({ 'position': 'fixed', 'top': `${bottomOfScreen}px`, });

brenniebrennie

Trailing whieespace.

brenniebrennie

Undo this.

brenniebrennie

Undo this.

brenniebrennie

Trailing whitespace.

brenniebrennie

Col: 10 Unnecessary semicolon.

reviewbotreviewbot

Col: 10 Unnecessary semicolon.

reviewbotreviewbot

You have some trailing whitespace over here.

bolariinwabolariinwa

There is some whitespace here

bolariinwabolariinwa

There is some whitespace here

bolariinwabolariinwa

Col: 17 'x' is defined but never used.

reviewbotreviewbot

Col: 17 'y' is defined but never used.

reviewbotreviewbot

You could chain these jQuery methods to keep the code a bit more concise like this: commentBox .on('mousdown', () => …

shovenshoven

Is there a reason you need both eTop and top? Is it possible to condense these two lines down to …

shovenshoven

Your comment makes it super clear what the two conditions are to return true, but based on the names of …

shovenshoven

Should this comment still be here?

skaefer143skaefer143

This should have one more space added to it, to keep it in line with the it() function.

skaefer143skaefer143

There should be a newline between these two lines.

skaefer143skaefer143

There should be a newline between these two lines.

skaefer143skaefer143
Sudolicious
Review request changed
Change Summary:

These are the things I have been experimenting, with keeping the Comment Dialog fixed to the page

Branch:
release-4.0.x
master
Commit:
9bbef83c1b4f2b33de5395d221e8ae24122f345a
0891a91f40b7cbbc3dd95691929a102f6c5952cd

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Sudolicious
Review request changed
Change Summary:

Trying out Mike's proposed change

Commit:
0891a91f40b7cbbc3dd95691929a102f6c5952cd
aa1d801491fb27f0c9273457395643590163c94a

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

alextechcc
  1. 
      
  2. Show all issues

    Most of these could be declared const.

    1. Thanks Alex! I'll update them

  3. 
      
Sudolicious
Review request changed
Change Summary:

So I found a bug that move the dialog box down by 8 pixels for some reason, I plan to look into that further.
I also contained the box to the document, since when the box would go into a negative top value it would break the
Comment dialog box.

Commit:
aa1d801491fb27f0c9273457395643590163c94a
4608534188725af0e232703dff229acc59f33ba3

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Sudolicious
Review request changed
Change Summary:

This change will fix the bugs I had when the cursor would leave the content area.

Commit:
4608534188725af0e232703dff229acc59f33ba3
cecbe4a2e51213f5f5a6716194e501379d730e92

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Sudolicious
brennie
  1. 
      
  2. Show all issues

    trailing whitespace.

  3. Show all issues

    This can just be .css('position', 'fixed').

    No need to construct an object.

    If you want to keep it as an object, space after :.

  4. Show all issues

    Trailing whitespace.

  5. Show all issues

    Only the comments before a function should be /**. Regular multiline comments can use /*.

    Same below.

    1. Are events that execute a function not considered functions?

    2. Don't worry about documenting the inline functions in event handlers.

  6. Show all issues

    Trailing whitespace.

    Missing space after period.

  7. Show all issues

    This can be a single computation.

    const top = commentBox.offset().top - 8;
    
  8. Show all issues

    What is 8 here? where does this come from?

    1. I updated the css this time in order to remedy that.

  9. Show all issues

    You can replace this with:

    commentBox.css({
        'position': 'absolute',
        'top': `${top}px`,
    });
    
  10. Show all issues

    This can be a single computation.

    Always space around binary operators.

  11. Show all issues

    The parentheses are redundant.

  12. Show all issues

    You can replace this with:

    commentBox.css({
        'position': 'fixed',
        'top': `${top}px`,
    });
    
  13. reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    Doc comments should be of the form:

    ```
    /*
    * Single line summary (in imperitive mood).
    * * Multi-line display.
    * * Returns:
    * returntype:
    * Description of return value.
    /

  14. Show all issues

    Trailing whitespace.

    Dialog -> dialog

    Checks -> Check.

  15. Show all issues

    We generally use function statements in this case, e.g.

    function isAbove() {
    //  ...
    }
    

    Same with isBelow below.

  16. Show all issues

    This should be named something like isTopPosition because it is a boolean. A variable named topPosition would be, e.g., the y coordinate of the top position.

  17. Show all issues

    Where does 8 come from?

  18. Show all issues

    Blank line between these.

  19. reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    Replace with:

    return topPosition || offsetTop === 0;
    
  20. reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    Doc comments should be of the form:

    ```
    /*
    * Single line summary (in imperitive mood).
    * * Multi-line display.
    * * Returns:
    * returntype:
    * Description of return value.
    /

  21. reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    Replace with:

    return commentBox.offset().top > bottomOfScreen;
    
  22. reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    Trailing whitespace.

  23. Show all issues

    stop: function(event, ui) {

  24. Show all issues

    You can replace this with:

    ui.helper.css({
        'position': 'fixed',
        'top': `${ui.helper.scrollTop()}px`,
    });
    
  25. Show all issues

    } else if (isBelow()) {

  26. Show all issues

    You can replace this with:

    ui.helper.css({
        'position': 'fixed',
        'top': `${bottomOfScreen}px`,
    });
    
  27. Show all issues

    Trailing whieespace.

  28. Show all issues

    Undo this.

  29. Show all issues

    Undo this.

  30. Show all issues

    Trailing whitespace.

  31. 
      
Sudolicious
Review request changed
Change Summary:

Addressing some of Barret's comments

Commit:
ce53ddcb39b6d8608385b6dcf36e28cdf46352df
fcf2cb57eaba3726d458ad8035ac8adecfbf1f20

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
  1. 
      
  2. Show all issues

    You have some trailing whitespace over here.

  3. 
      
Sudolicious
Review request changed
Change Summary:

Added Unit Tests and updated comments

Testing Done:
  +

Ran and passed all of the JavaScript Unit Tests.

  +
  +

I created a test to check whether the position was being set

  + correctly on mousedown and mouseup.

Commit:
fcf2cb57eaba3726d458ad8035ac8adecfbf1f20
e737bc2b5c2a364e858a937f3d7f0055b5081313

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
  1. 
      
  2. Show all issues

    There is some whitespace here

  3. Show all issues

    There is some whitespace here

  4. 
      
Sudolicious
skaefer143
  1. 
      
  2. Show all issues

    Should this comment still be here?

  3. Show all issues

    This should have one more space added to it, to keep it in line with the it() function.

  4. Show all issues

    There should be a newline between these two lines.

  5. Show all issues

    There should be a newline between these two lines.

  6. 
      
shoven
  1. 
      
  2. Show all issues

    You could chain these jQuery methods to keep the code a bit more concise like this:

    commentBox
        .on('mousdown', () => {...})
        .on('mouseup', () => {...})
    
  3. Show all issues

    Is there a reason you need both eTop and top? Is it possible to condense these two lines down to const top = commentBox.offset().top?

  4. Show all issues

    Your comment makes it super clear what the two conditions are to return true, but based on the names of these variables it's not super clear. Maybe you could try isTopPosition -> isOutsideContentArea and isOffsetTop -> isAtTopOfDocument (or something like that)?

  5. 
      
Sudolicious
Review request changed
Change Summary:

Addressing the changes brought up by Sarah, Storm and Bola

Commit:
8bc59bf5607d752694c6e28781467783d7d0240b
7b001f41308352652c5c6d5491192fa6c1139777

Checks run (2 succeeded)

flake8 passed.
JSHint passed.