Docking the Comment Dialog

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

Sudolicious
Review Board
master
7b001f4...
reviewboard, students

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.

  • 0
  • 0
  • 60
  • 0
  • 60
Description From Last Updated
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

Diff:

Revision 2 (+59 -6)

Show changes

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

Diff:

Revision 3 (+41 -6)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

alextechcc
  1. 
      
  2. 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

Diff:

Revision 4 (+68 -6)

Show changes

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

Diff:

Revision 5 (+58 -7)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Sudolicious
brennie
  1. 
      
  2. trailing whitespace.

  3. 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. Trailing whitespace.

  5. 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. Trailing whitespace.

    Missing space after period.

  7. This can be a single computation.

    const top = commentBox.offset().top - 8;
    
  8. What is 8 here? where does this come from?

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

  9. You can replace this with:

    commentBox.css({
        'position': 'absolute',
        'top': `${top}px`,
    });
    
  10. This can be a single computation.

    Always space around binary operators.

  11. The parentheses are redundant.

  12. You can replace this with:

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

    Doc comments should be of the form:

    ```
    /
    * Single line summary (in imperitive mood).

    * Multi-line display.

    * Returns:
    * returntype:
    * Description of return value.
    */

  14. Trailing whitespace.

    Dialog -> dialog

    Checks -> Check.

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

    function isAbove() {
    //  ...
    }
    

    Same with isBelow below.

  16. 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. Where does 8 come from?

  18. Blank line between these.

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

    Replace with:

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

    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)
     
     
     
     
     
     
     
     

    Replace with:

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

    Trailing whitespace.

  23. stop: function(event, ui) {

  24. You can replace this with:

    ui.helper.css({
        'position': 'fixed',
        'top': `${ui.helper.scrollTop()}px`,
    });
    
  25. } else if (isBelow()) {

  26. You can replace this with:

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

  28. Trailing whitespace.

  29. 
      
Sudolicious
Review request changed

Change Summary:

Addressing some of Barret's comments

Commit:

-ce53ddcb39b6d8608385b6dcf36e28cdf46352df
+fcf2cb57eaba3726d458ad8035ac8adecfbf1f20

Diff:

Revision 7 (+81 -9)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
  1. 
      
  2. 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

Diff:

Revision 8 (+92 -10)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
  1. 
      
  2. There is some whitespace here

  3. There is some whitespace here

  4. 
      
Sudolicious
skaefer143
  1. 
      
  2. Should this comment still be here?

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

  4. There should be a newline between these two lines.

  5. There should be a newline between these two lines.

  6. 
      
shoven
  1. 
      
  2. You could chain these jQuery methods to keep the code a bit more concise like this:

    commentBox
        .on('mousdown', () => {...})
        .on('mouseup', () => {...})
    
  3. 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. 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

Diff:

Revision 10 (+77 -9)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...