Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Docking the Comment Dialog
Review Request #10213 — Created Oct. 10, 2018 and updated
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 onmousedownandmouseup.
| Description | From | Last Updated |
|---|---|---|
|
Col: 13 'newTop' is defined but never used. |
reviewbot
|
|
|
Col: 53 Missing semicolon. |
reviewbot
|
|
|
Col: 18 'getTop' is defined but never used. |
reviewbot
|
|
|
Col: 34 Missing semicolon. |
reviewbot
|
|
|
Col: 31 Missing semicolon. |
reviewbot
|
|
|
Most of these could be declared const. |
alextechcc
|
|
|
Col: 18 'getTop' is defined but never used. |
reviewbot
|
|
|
Col: 34 Missing semicolon. |
reviewbot
|
|
|
Col: 31 Missing semicolon. |
reviewbot
|
|
|
Col: 18 'getTop' is defined but never used. |
reviewbot
|
|
|
Col: 34 Missing semicolon. |
reviewbot
|
|
|
Col: 31 Missing semicolon. |
reviewbot
|
|
|
Col: 71 Missing semicolon. |
reviewbot
|
|
|
Col: 41 Missing semicolon. |
reviewbot
|
|
|
Col: 15 'isAbove' was used before it was defined. |
reviewbot
|
|
|
Col: 10 Missing semicolon. |
reviewbot
|
|
|
Col: 31 Missing semicolon. |
reviewbot
|
|
|
trailing whitespace. |
brennie
|
|
|
This can just be .css('position', 'fixed'). No need to construct an object. If you want to keep it as an … |
brennie
|
|
|
Trailing whitespace. |
brennie
|
|
|
Only the comments before a function should be /**. Regular multiline comments can use /*. Same below. |
brennie
|
|
|
Trailing whitespace. Missing space after period. |
brennie
|
|
|
This can be a single computation. const top = commentBox.offset().top - 8; |
brennie
|
|
|
What is 8 here? where does this come from? |
brennie
|
|
|
You can replace this with: commentBox.css({ 'position': 'absolute', 'top': `${top}px`, }); |
brennie
|
|
|
This can be a single computation. Always space around binary operators. |
brennie
|
|
|
The parentheses are redundant. |
brennie
|
|
|
You can replace this with: commentBox.css({ 'position': 'fixed', 'top': `${top}px`, }); |
brennie
|
|
|
Doc comments should be of the form: ``` /* * Single line summary (in imperitive mood). * * Multi-line display. … |
brennie
|
|
|
Trailing whitespace. Dialog -> dialog Checks -> Check. |
brennie
|
|
|
We generally use function statements in this case, e.g. function isAbove() { // ... } Same with isBelow below. |
brennie
|
|
|
This should be named something like isTopPosition because it is a boolean. A variable named topPosition would be, e.g., the … |
brennie
|
|
|
Where does 8 come from? |
brennie
|
|
|
Blank line between these. |
brennie
|
|
|
Replace with: return topPosition || offsetTop === 0; |
brennie
|
|
|
Doc comments should be of the form: ``` /* * Single line summary (in imperitive mood). * * Multi-line display. … |
brennie
|
|
|
Replace with: return commentBox.offset().top > bottomOfScreen; |
brennie
|
|
|
Trailing whitespace. |
brennie
|
|
|
stop: function(event, ui) { |
brennie
|
|
|
You can replace this with: ui.helper.css({ 'position': 'fixed', 'top': `${ui.helper.scrollTop()}px`, }); |
brennie
|
|
|
} else if (isBelow()) { |
brennie
|
|
|
You can replace this with: ui.helper.css({ 'position': 'fixed', 'top': `${bottomOfScreen}px`, }); |
brennie
|
|
|
Trailing whieespace. |
brennie
|
|
|
Undo this. |
brennie
|
|
|
Undo this. |
brennie
|
|
|
Trailing whitespace. |
brennie
|
|
|
Col: 10 Unnecessary semicolon. |
reviewbot
|
|
|
Col: 10 Unnecessary semicolon. |
reviewbot
|
|
|
You have some trailing whitespace over here. |
bolariinwa
|
|
|
There is some whitespace here |
bolariinwa
|
|
|
There is some whitespace here |
bolariinwa
|
|
|
Col: 17 'x' is defined but never used. |
reviewbot
|
|
|
Col: 17 'y' is defined but never used. |
reviewbot
|
|
|
You could chain these jQuery methods to keep the code a bit more concise like this: commentBox .on('mousdown', () => … |
shoven
|
|
|
Is there a reason you need both eTop and top? Is it possible to condense these two lines down to … |
shoven
|
|
|
Your comment makes it super clear what the two conditions are to return true, but based on the names of … |
shoven
|
|
|
Should this comment still be here? |
skaefer143
|
|
|
This should have one more space added to it, to keep it in line with the it() function. |
skaefer143
|
|
|
There should be a newline between these two lines. |
skaefer143
|
|
|
There should be a newline between these two lines. |
skaefer143
|
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.xmaster
- Commit:
-
9bbef83c1b4f2b33de5395d221e8ae24122f345a0891a91f40b7cbbc3dd95691929a102f6c5952cd
Review request changed
- Change Summary:
-
Trying out Mike's proposed change
- Commit:
-
0891a91f40b7cbbc3dd95691929a102f6c5952cdaa1d801491fb27f0c9273457395643590163c94a
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 negativetopvalue it would break the
Comment dialog box. - Commit:
-
aa1d801491fb27f0c9273457395643590163c94a4608534188725af0e232703dff229acc59f33ba3
Review request changed
- Change Summary:
-
This change will fix the bugs I had when the cursor would leave the content area.
- Commit:
-
4608534188725af0e232703dff229acc59f33ba3cecbe4a2e51213f5f5a6716194e501379d730e92
Review request changed
- 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: absolutewas being set using jQuery.~ So I essentially updated the jQuery in order to have the dialog's ~ position:fixedwhen scrolling. I also used jQuery to assign a~ topvalue 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:
-
cecbe4a2e51213f5f5a6716194e501379d730e92ce53ddcb39b6d8608385b6dcf36e28cdf46352df
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
This can just be
.css('position', 'fixed').No need to construct an object.
If you want to keep it as an object, space after
:. -
-
Only the comments before a function should be
/**. Regular multiline comments can use/*.Same below.
-
-
-
-
-
-
-
-
Doc comments should be of the form:
```
/*
* Single line summary (in imperitive mood).
*
* Multi-line display.
*
* Returns:
* returntype:
* Description of return value.
/ -
-
We generally use
functionstatements in this case, e.g.function isAbove() { // ... }Same with
isBelowbelow. -
This should be named something like
isTopPositionbecause it is a boolean. A variable namedtopPositionwould be, e.g., the y coordinate of the top position. -
-
-
-
Doc comments should be of the form:
```
/*
* Single line summary (in imperitive mood).
*
* Multi-line display.
*
* Returns:
* returntype:
* Description of return value.
/ -
-
-
-
You can replace this with:
ui.helper.css({ 'position': 'fixed', 'top': `${ui.helper.scrollTop()}px`, }); -
-
-
-
-
-
Review request changed
- Change Summary:
-
Addressing some of Barret's comments
- Commit:
-
ce53ddcb39b6d8608385b6dcf36e28cdf46352dffcf2cb57eaba3726d458ad8035ac8adecfbf1f20
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 mousedownandmouseup. - Commit:
-
fcf2cb57eaba3726d458ad8035ac8adecfbf1f20e737bc2b5c2a364e858a937f3d7f0055b5081313
Review request changed
- Change Summary:
-
Removing WIP tag, and editing some of the tests
- Summary:
-
[WIP] Docking the Comment DialogDocking 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 mousedownandmouseup. - Commit:
-
e737bc2b5c2a364e858a937f3d7f0055b50813138bc59bf5607d752694c6e28781467783d7d0240b
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
You could chain these jQuery methods to keep the code a bit more concise like this:
commentBox .on('mousdown', () => {...}) .on('mouseup', () => {...}) -
Is there a reason you need both
eTopandtop? Is it possible to condense these two lines down toconst top = commentBox.offset().top? -
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->isOutsideContentAreaandisOffsetTop->isAtTopOfDocument(or something like that)?