Summary

Docking the Comment Dialog

Review Request #10213 — Created Oct. 10, 2018 and updated Dec. 14, 2018, 2:10 p.m.

Information

Owner

Sudolicious

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

7b001f4...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Col: 13 'newTop' is defined but never used.	reviewbot	Nov. 7, 2018, 3:30 p.m.
Col: 53 Missing semicolon.	reviewbot	Nov. 7, 2018, 3:30 p.m.
Col: 18 'getTop' is defined but never used.	reviewbot	Nov. 7, 2018, 3:30 p.m.
Col: 34 Missing semicolon.	reviewbot	Nov. 7, 2018, 3:31 p.m.
Col: 31 Missing semicolon.	reviewbot	Nov. 7, 2018, 3:30 p.m.
Most of these could be declared const.	alextechcc	Nov. 7, 2018, 3:31 p.m.
Col: 18 'getTop' is defined but never used.	reviewbot	Nov. 7, 2018, 3:31 p.m.
Col: 34 Missing semicolon.	reviewbot	Nov. 7, 2018, 3:31 p.m.
Col: 31 Missing semicolon.	reviewbot	Nov. 7, 2018, 3:31 p.m.
Col: 18 'getTop' is defined but never used.	reviewbot	Nov. 7, 2018, 3:32 p.m.
Col: 34 Missing semicolon.	reviewbot	Nov. 7, 2018, 3:32 p.m.
Col: 31 Missing semicolon.	reviewbot	Nov. 7, 2018, 3:32 p.m.
Col: 71 Missing semicolon.	reviewbot	Nov. 10, 2018, 9:41 a.m.
Col: 41 Missing semicolon.	reviewbot	Nov. 10, 2018, 9:41 a.m.
Col: 15 'isAbove' was used before it was defined.	reviewbot	Nov. 10, 2018, 9:41 a.m.
Col: 10 Missing semicolon.	reviewbot	Nov. 10, 2018, 6:01 p.m.
Col: 31 Missing semicolon.	reviewbot	Nov. 7, 2018, 3:31 p.m.
trailing whitespace.	brennie	Nov. 17, 2018, 12:05 p.m.
This can just be .css('position', 'fixed'). No need to construct an object. If you want to keep it as an …	brennie	Nov. 17, 2018, 12:05 p.m.
Trailing whitespace.	brennie	Nov. 17, 2018, 12:06 p.m.
Only the comments before a function should be /*. Regular multiline comments can use /. Same below.	brennie	Dec. 8, 2018, 3 p.m.
Trailing whitespace. Missing space after period.	brennie	Nov. 17, 2018, 12:07 p.m.
This can be a single computation. const top = commentBox.offset().top - 8;	brennie	Nov. 17, 2018, 12:07 p.m.
What is 8 here? where does this come from?	brennie	Nov. 17, 2018, 12:07 p.m.
You can replace this with: commentBox.css({ 'position': 'absolute', 'top': `${top}px`, });	brennie	Nov. 17, 2018, 12:08 p.m.
This can be a single computation. Always space around binary operators.	brennie	Nov. 17, 2018, 12:08 p.m.
The parentheses are redundant.	brennie	Nov. 17, 2018, 12:10 p.m.
You can replace this with: commentBox.css({ 'position': 'fixed', 'top': `${top}px`, });	brennie	Nov. 17, 2018, 12:23 p.m.
Doc comments should be of the form: ``` /* * Single line summary (in imperitive mood). * * Multi-line display. …	brennie	Dec. 8, 2018, 3 p.m.
Trailing whitespace. Dialog -> dialog Checks -> Check.	brennie	Nov. 17, 2018, 12:24 p.m.
We generally use function statements in this case, e.g. function isAbove() { // ... } Same with isBelow below.	brennie	Nov. 17, 2018, 12:24 p.m.
This should be named something like isTopPosition because it is a boolean. A variable named topPosition would be, e.g., the …	brennie	Nov. 17, 2018, 12:24 p.m.
Where does 8 come from?	brennie	Nov. 17, 2018, 12:24 p.m.
Blank line between these.	brennie	Nov. 17, 2018, 12:25 p.m.
Replace with: return topPosition \|\| offsetTop === 0;	brennie	Nov. 17, 2018, 12:25 p.m.
Doc comments should be of the form: ``` /* * Single line summary (in imperitive mood). * * Multi-line display. …	brennie	Dec. 8, 2018, 3 p.m.
Replace with: return commentBox.offset().top > bottomOfScreen;	brennie	Nov. 17, 2018, 12:25 p.m.
Trailing whitespace.	brennie	Nov. 17, 2018, 12:26 p.m.
stop: function(event, ui) {	brennie	Nov. 17, 2018, 12:25 p.m.
You can replace this with: ui.helper.css({ 'position': 'fixed', 'top': `${ui.helper.scrollTop()}px`, });	brennie	Nov. 17, 2018, 12:26 p.m.
} else if (isBelow()) {	brennie	Nov. 17, 2018, 12:27 p.m.
You can replace this with: ui.helper.css({ 'position': 'fixed', 'top': `${bottomOfScreen}px`, });	brennie	Nov. 17, 2018, 12:27 p.m.
Trailing whieespace.	brennie	Nov. 17, 2018, 12:29 p.m.
Undo this.	brennie	Nov. 17, 2018, 12:32 p.m.
Undo this.	brennie	Nov. 17, 2018, 12:32 p.m.
Trailing whitespace.	brennie	Nov. 17, 2018, 12:30 p.m.
Col: 10 Unnecessary semicolon.	reviewbot	Nov. 17, 2018, 12:40 p.m.
Col: 10 Unnecessary semicolon.	reviewbot	Nov. 17, 2018, 12:40 p.m.
You have some trailing whitespace over here.	bolariinwa	Dec. 8, 2018, 3 p.m.
There is some whitespace here	bolariinwa	Dec. 10, 2018, 12:45 p.m.
There is some whitespace here	bolariinwa	Dec. 14, 2018, 2:05 p.m.
Col: 17 'x' is defined but never used.	reviewbot	Dec. 8, 2018, 3:19 p.m.
Col: 17 'y' is defined but never used.	reviewbot	Dec. 8, 2018, 3:19 p.m.
You could chain these jQuery methods to keep the code a bit more concise like this: commentBox .on('mousdown', () => …	shoven	Dec. 14, 2018, 2:02 p.m.
Is there a reason you need both eTop and top? Is it possible to condense these two lines down to …	shoven	Dec. 14, 2018, 12:45 p.m.
Your comment makes it super clear what the two conditions are to return true, but based on the names of …	shoven	Dec. 14, 2018, 2:03 p.m.
Should this comment still be here?	skaefer143	Dec. 14, 2018, 12:42 p.m.
This should have one more space added to it, to keep it in line with the it() function.	skaefer143	Dec. 14, 2018, 12:43 p.m.
There should be a newline between these two lines.	skaefer143	Dec. 14, 2018, 12:43 p.m.
There should be a newline between these two lines.	skaefer143	Dec. 14, 2018, 12:44 p.m.

flake8 passed.

JSHint passed.

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

reviewboard/static/rb/js/views/commentDialogView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 2)
Show all issues
```
Col: 13
 'newTop' is defined but never used.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 2)
Show all issues
```
Col: 53
 Missing semicolon.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 2)
Show all issues
```
Col: 18
 'getTop' is defined but never used.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 2)
Show all issues
```
Col: 34
 Missing semicolon.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 2)
Show all issues
```
Col: 31
 Missing semicolon.
```

Change Summary:

Trying out Mike's proposed change

Commit:

-	0891a91f40b7cbbc3dd95691929a102f6c5952cd
+	aa1d801491fb27f0c9273457395643590163c94a

Diff:

Revision 3 (+41 -6)

Show changes

reviewboard/static/rb/js/views/commentDialogView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 3)
Show all issues
```
Col: 18
 'getTop' is defined but never used.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 3)
Show all issues
```
Col: 34
 Missing semicolon.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 3)
Show all issues
```
Col: 31
 Missing semicolon.
```

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 3)
Show all issues
```
Most of these could be declared const.
```
1. Sudolicious Oct. 27, 2018, 12:47 p.m.
  Thanks Alex! I'll update them

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

reviewboard/static/rb/js/views/commentDialogView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 4)
Show all issues
```
Col: 18
 'getTop' is defined but never used.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 4)
Show all issues
```
Col: 34
 Missing semicolon.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 4)
Show all issues
```
Col: 31
 Missing semicolon.
```

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

reviewboard/static/rb/js/views/commentDialogView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 5)
Show all issues
```
Col: 71
 Missing semicolon.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 5)
Show all issues
```
Col: 41
 Missing semicolon.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 5)
Show all issues
```
Col: 15
 'isAbove' was used before it was defined.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 5)
Show all issues
```
Col: 10
 Missing semicolon.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 5)
Show all issues
```
Col: 31
 Missing semicolon.
```

Change Summary:

Updated the description and the Summary.

Summary:

-	[WIP] Docking the Comment Dialog when scrolling
+	[WIP] Docking the Comment Dialog

Description:

		Comment Dialog: Docking

~		After digging around in the reviews.less, and commentDialogView.es6.js
~		files I found that `property: absolute` was being set using jQuery.
~		So I essentially updated the jQuery in order to have the dialog's
~		`position:fixed` when scrolling. I also used jQuery to assign a
~		`top` value since the comment dialog would not appear otherwise.
	~	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.

~		But now I have created a bug where when dragging the comment dialog
~		to change the position, when used to scroll along the page the dialog
~		box will run away from the cursor. This bug doesn't appear when
~		`position: absolute`.
~
	~	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.
-		My current theory is that the method used to affect the dialog's `top`
-		property, when dragging the dialog box, is what causes the bug.
-		This is a rough theory that and I am not entirely sure it's correct.
-		I am also unsure if I should use jQuery to affect the `position`
-		property since I feel like this doesn't preserve a
-		"separation of concerns".

Commit:

-	cecbe4a2e51213f5f5a6716194e501379d730e92
+	ce53ddcb39b6d8608385b6dcf36e28cdf46352df

Diff:

Revision 6 (+89 -8)

Show changes

reviewboard/static/rb/js/views/commentDialogView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

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

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 :.

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

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

Show all issues

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

Sudolicious Nov. 17, 2018, 12:32 p.m.

Are events that execute a function not considered functions?

brennie Nov. 24, 2018, 3 p.m.

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

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
Trailing whitespace.
Missing space after period.
```

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

Show all issues

This can be a single computation.

const top = commentBox.offset().top - 8;

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
What is 8 here? where does this come from?
```
1. Sudolicious Nov. 17, 2018, 12:32 p.m.
  I updated the css this time in order to remedy that.

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

Show all issues

You can replace this with:

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

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

Show all issues

This can be a single computation.

Always space around binary operators.

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
The parentheses are redundant.
```

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

Show all issues

You can replace this with:

commentBox.css({
    'position': 'fixed',
    'top': `${top}px`,
});

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.

/

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
Trailing whitespace.
Dialog -> dialog
Checks -> Check.
```

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

Show all issues

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

function isAbove() {
//  ...
}


Same with isBelow below.

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

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.

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
Where does 8 come from?
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
Blank line between these.
```

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

Show all issues

Replace with:

return topPosition || offsetTop === 0;

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.

/

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

Show all issues

Replace with:

return commentBox.offset().top > bottomOfScreen;

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

Show all issues

Trailing whitespace.

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
stop: function(event, ui) {
```

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

Show all issues

You can replace this with:

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

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
} else if (isBelow()) {
```

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

Show all issues

You can replace this with:

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

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
Trailing whieespace.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
Undo this.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
Undo this.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 6)
Show all issues
```
Trailing whitespace.
```

Change Summary:

Addressing some of Barret's comments

Commit:

-	ce53ddcb39b6d8608385b6dcf36e28cdf46352df
+	fcf2cb57eaba3726d458ad8035ac8adecfbf1f20

Diff:

Revision 7 (+81 -9)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/commentDialogView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 7)
Show all issues
```
Col: 10
 Unnecessary semicolon.
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 7)
Show all issues
```
Col: 10
 Unnecessary semicolon.
```

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 7)
Show all issues
```
You have some trailing whitespace over here.
```

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

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/commentDialogView.es6.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 8)
Show all issues
```
Col: 17
 'x' is defined but never used.
```
reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 8)
Show all issues
```
Col: 17
 'y' is defined but never used.
```

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 8)
Show all issues
```
There is some whitespace here
```
reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 8)
Show all issues
```
There is some whitespace here
```

Change Summary:

Removing WIP tag, and editing some of the tests

Summary:

-	[WIP] Docking the Comment Dialog
+	Docking the Comment Dialog

Testing Done:

		Ran and passed all of the JavaScript Unit Tests.

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

Commit:

-	e737bc2b5c2a364e858a937f3d7f0055b5081313
+	8bc59bf5607d752694c6e28781467783d7d0240b

Diff:

Revision 9 (+76 -10)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/commentDialogView.es6.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/views/commentDialogView.es6.js (Diff revision 9)
Show all issues
```
Should this comment still be here?
```

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 9)

Show all issues

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

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 9)
Show all issues
```
There should be a newline between these two lines.
```
reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 9)
Show all issues
```
There should be a newline between these two lines.
```

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

Show all issues

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

commentBox
    .on('mousdown', () => {...})
    .on('mouseup', () => {...})

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

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?

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

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)?

Change Summary:

Addressing the changes brought up by Sarah, Storm and Bola

Commit:

-	8bc59bf5607d752694c6e28781467783d7d0240b
+	7b001f41308352652c5c6d5491192fa6c1139777

Diff:

Revision 10 (+77 -9)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/commentDialogView.es6.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.