Summary

80- (or n-) column indicator in diff viewer

Review Request #11294 — Created Nov. 20, 2020 and updated Dec. 6, 2020, 10:50 p.m.

Information

Owner

keanweng

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

The current diff viewer does not have a column indicator that shows
the character boundary. Adding the feature will allow reviewers to see
when a line exceeds n characters.

We added a red dashed line on the diff viewer at the specified column.
The indicator defaults to 80 characters and the user can specify a
number at the input box on top. With file changes, the indicator shows
on both side of the diff viewer and changes together. We also check
for the maximum column number based on the diff viewer's size. If the
diff viewer is shrunk beyond the indicator, the indicator will be
hidden.

We added the indicator as a div which the CSS property
border-left: 1px dashed red. Whenever the column number is changed,
moveColumnIndicator() does the calculation and moves the indicator
to the new position with jQuery's css(). Although JS function does
the calculations everytime the position changes, in theory, a user
will not be changing the position too often.

Testing Done:

Tested following behaviours:

Set column indicator to 0 (check underflow)
Set column indicator to 999 (check overflow)
Check indicator line

Browsers tested:

Chrome (Version 87.0.4280)
Edge (Version 87.0.664.5)
Firefox (Version 83.0)
Safari (Version 14.0.1)

Testing Done

Tested following behaviours:

Set column indicator to 0 (check underflow)
Set column indicator to 999 (check overflow)
Check indicator line

Browsers tested:

Chrome (Version 87.0.4280)
Edge (Version 87.0.664.5)
Firefox (Version 83.0)
Safari (Version 14.0.1)

Commits

Summary
	The current diff viewer does not have a column indicator that shows the

Files

Issues

Description	From	Last Updated
I tried hosting the patched server on my local machine and the column indicator looks great! I agree with Jacob …	bnie	Dec. 4, 2020, 7:06 p.m.
It looks like some lines in the description are longer than the format expected. Could you target ~70 (upwards of …	ceciliawei	Dec. 6, 2020, 10:49 p.m.
When referencing a JavaScript method in the docs, you'll want to use the js:meth: role, like: :js:meth:`_moveColumnIndicator` Same with docs …	chipx86	Dec. 3, 2020, 5:42 p.m.
Rather than var, this should use const. Or just pass the expression into the call that needs it. I would …	chipx86	Dec. 3, 2020, 5:50 p.m.
You can define these where they're needed. Either const or let should be used instead of var, depending on whether …	chipx86	Dec. 3, 2020, 5:51 p.m.
Let's use .text(), to better communicate that we're not inserting HTML.	chipx86	Dec. 3, 2020, 6:34 p.m.
Where does this number come from?	chipx86	Dec. 4, 2020, 9:51 p.m.
Blank line between statements that form blocks.	chipx86	Dec. 3, 2020, 6:08 p.m.
The - operator should go on the previous line, and each line of the expression should be aligned.	chipx86	Dec. 3, 2020, 6:15 p.m.
Each parameter to Math.min() should be aligned.	chipx86	Dec. 3, 2020, 6:15 p.m.
You can use template literals here and above: `translateX(${columnNum * charWidth}px)`	chipx86	Dec. 3, 2020, 6:34 p.m.

flake8 passed.

JSHint passed.

Commits:

	Summary
-	Add indicator line.
+	Add indicator line.
+	Add input to switch n-column.

Diff:

Revision 2 (+70 -2)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Add Description.

Description:

~		Add indicator line.
	~	The current diff viewer does not have a column indicator that shows the
	+	character boundary. Adding the feature will allow reviewers to see when a line
	+	exceeds n characters.
	+
	+	We added a red dashed line on the diff viewer at the specified column. The
	+	indicator defaults to 80 characters and the user can specify a number at the
	+	input box on top. With file changes, the indicator shows on both side of the
	+	diff viewer and changes together. We also check for the maximum column number
	+	based on the diff viewer's size. If the diff viewer is shrunk beyond the
	+	indicator, the indicator will be hidden.
	+
	+	We added the indicator as a `div` which the CSS property
	+	`border-left: 1px dashed red`. Whenever the column number is changed,
	+	`moveColumnIndicator()` does the calculation and moves the indicator to the new
	+	position with jQuery's `css()`. Although JS function does the calculations
	+	everytime the position changes, in theory, a user will not be changing the
	+	position too often.

Commits:

	Summary
-		Add indicator line.
-		Add input to switch n-column.
+		Add indicator line.
+		Add input to switch n-column.
+		Update indicator to div, working.
+		The current diff viewer does not have a column indicator that shows the

Groups:

+	students

Diff:

Revision 3 (+251 -33)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Removed Files:
Added Files:

Change Summary:

Add screenshots from different browsers.

Added Files:

Change Summary:

Clean up commits.

Testing Done:

	+	Tested following behaviours:
	+
	+	Set column indicator to 0 (check underflow)
	+	Set column indicator to 999 (check overflow)
	+	Check indicator line
	+
	+	Browsers tested:
	+
	+	Chrome (Version 87.0.4280)
	+	Edge (Version 87.0.664.5)
	+	Firefox (Version 83.0)
	+	Safari (Version 14.0.1)

Commits:

	Summary
-		Add indicator line.
-		Add input to switch n-column.
-		Update indicator to div, working.
-		The current diff viewer does not have a column indicator that shows the
+		The current diff viewer does not have a column indicator that shows the

Diff:

Revision 4 (+222 -4)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Remove WIP tag

Summary:

-	[WIP] 80- (or n-) column indicator in diff viewer
+	80- (or n-) column indicator in diff viewer

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)

Show all issues

When referencing a JavaScript method in the docs, you'll want to use the js:meth: role, like:

:js:meth:`_moveColumnIndicator`

Same with docs below.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)

Show all issues

Rather than var, this should use const. Or just pass the expression into the call that needs it.

I would rework this to properly use the chaining syntax:

cons newColumnNum =
    this.$el
    .find('....')
    .val()

That's often used with jQuery, and it helps make each step in the chain clear.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)

Show all issues

You can define these where they're needed. Either const or let should be used instead of var, depending on whether the variable needs to be reassigned.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)
Show all issues
```
Let's use .text(), to better communicate that we're not inserting HTML.
```

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)

Show all issues

Where does this number come from?

keanweng Dec. 3, 2020, 6:37 p.m.

This is the value of one ch in my Chrome browser, it's different across browsers as they render the font sizes differently. It shouldn't affect much as this value is only used to move the input box on deleted files in the diff viewer (deleted files does not show the column indicator by default, so there is no way to measure ch). This is so that the user can still move the input box, although it won't be 100% accurate.

keanweng Dec. 4, 2020, 7:07 p.m.

The updated version does not require this value anymore since the input box will not be moving.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)
Show all issues
```
Blank line between statements that form blocks.
```

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)

Show all issues

The - operator should go on the previous line, and each line of the expression should be aligned.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)
Show all issues
```
Each parameter to Math.min() should be aligned.
```

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 4)

Show all issues

You can use template literals here and above:

`translateX(${columnNum * charWidth}px)`

Change Summary:

Address feedback (fix styling).

Commits:

	Summary
-		The current diff viewer does not have a column indicator that shows the
+		The current diff viewer does not have a column indicator that shows the

Diff:

Revision 5 (+234 -4)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

I pulled down the patch, ran it locally, and it works great. Awesome work Kean! Not sure that I am too familiar with the code, so I will comment mostly on my user experience. Firstly, I noticed that the column indicator input text can overlap with the New File Download button if set to 0, this could be annoying especially for users that are uninterested in seeing the column indicator. In addition, I think that it could be useful to have a toggle button on the indicator to turn it on/off and give users more flexibility. I also noticed that the input field works with hexadecimal numbers such as 0xf, not sure if that is the intended behavior. But it's kind of cool nonetheless.

keanweng Dec. 4, 2020, 7:09 p.m.

Thanks for checking it out, Jacob! The input box now has a fixed position. I like that suggestion, I will include it in my future TODO's.

Show all issues

I tried hosting the patched server on my local machine and the column indicator looks great!
I agree with Jacob on the part when user sets the column indicator to around 5, the indicator overlaps with the text below it.
Also, I tested the diff viewer on both Chrome Version 86 and Safari 14. The indicator works perfectly on Chrome but on Safari, the indicator text box does not align with the indicator line, I've attached a picture below. This only happends when you first load the page and if you set it to something else and set it back later, the text box does align with the line indicator.

keanweng Dec. 4, 2020, 7:07 p.m.

Thanks for noticing this bug, Bruce! The input box's position is now fixed. (details)

Change Summary:

Change input box to fixed position and update screenshot. (details)

Commits:

	Summary
-		The current diff viewer does not have a column indicator that shows the
+		The current diff viewer does not have a column indicator that shows the

Diff:

Revision 6 (+194 -4)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Update screenshot.

Removed Files:

Show all issues

It looks like some lines in the description are longer than the format expected. Could you target ~70 (upwards of 75) characters for the wrap point?

keanweng Dec. 6, 2020, 10:50 p.m.

I've updated it to wrap at 70 characters, thanks for catching that!

Change Summary:

Update description to wrap at 70 columns.

Description:

~		The current diff viewer does not have a column indicator that shows the
~		character boundary. Adding the feature will allow reviewers to see when a line
~		exceeds n characters.
	~	The current diff viewer does not have a column indicator that shows
	~	the character boundary. Adding the feature will allow reviewers to see
	~	when a line exceeds n characters.

~		We added a red dashed line on the diff viewer at the specified column. The
~		indicator defaults to 80 characters and the user can specify a number at the
~		input box on top. With file changes, the indicator shows on both side of the
~		diff viewer and changes together. We also check for the maximum column number
~		based on the diff viewer's size. If the diff viewer is shrunk beyond the
~		indicator, the indicator will be hidden.
	~	We added a red dashed line on the diff viewer at the specified column.
	~	The indicator defaults to 80 characters and the user can specify a
	~	number at the input box on top. With file changes, the indicator shows
	~	on both side of the diff viewer and changes together. We also check
	~	for the maximum column number based on the diff viewer's size. If the
	~	diff viewer is shrunk beyond the indicator, the indicator will be
	+	hidden.

		We added the indicator as a `div` which the CSS property
		`border-left: 1px dashed red`. Whenever the column number is changed,
~		`moveColumnIndicator()` does the calculation and moves the indicator to the new
~		position with jQuery's `css()`. Although JS function does the calculations
~		everytime the position changes, in theory, a user will not be changing the
~		position too often.
	~	`moveColumnIndicator()` does the calculation and moves the indicator
	~	to the new position with jQuery's `css()`. Although JS function does
	~	the calculations everytime the position changes, in theory, a user
	~	will not be changing the position too often.
	+
	+	Testing Done:
	+
	+	Tested following behaviours:
	+	* Set column indicator to 0 (check underflow)
	+	* Set column indicator to 999 (check overflow)
	+	* Check indicator line
	+
	+	Browsers tested:
	+	* Chrome (Version 87.0.4280)
	+	* Edge (Version 87.0.664.5)
	+	* Firefox (Version 83.0)
	+	* Safari (Version 14.0.1)

Change Summary:

Update description.

Description:

		The current diff viewer does not have a column indicator that shows
		the character boundary. Adding the feature will allow reviewers to see
		when a line exceeds n characters.

		We added a red dashed line on the diff viewer at the specified column.
		The indicator defaults to 80 characters and the user can specify a
		number at the input box on top. With file changes, the indicator shows
		on both side of the diff viewer and changes together. We also check
		for the maximum column number based on the diff viewer's size. If the
		diff viewer is shrunk beyond the indicator, the indicator will be
		hidden.

		We added the indicator as a `div` which the CSS property
		`border-left: 1px dashed red`. Whenever the column number is changed,
		`moveColumnIndicator()` does the calculation and moves the indicator
		to the new position with jQuery's `css()`. Although JS function does
		the calculations everytime the position changes, in theory, a user
		will not be changing the position too often.

		Testing Done:

~		Tested following behaviours:
	~	Tested following behaviours:
-		* Set column indicator to 0 (check underflow)
-		* Set column indicator to 999 (check overflow)
-		* Check indicator line

~		Browsers tested:
~		* Chrome (Version 87.0.4280)
~		* Edge (Version 87.0.664.5)
~		* Firefox (Version 83.0)
~		* Safari (Version 14.0.1)
	~	Set column indicator to 0 (check underflow)
	~	Set column indicator to 999 (check overflow)
	~	Check indicator line
	~
	~	Browsers tested:
	+
	+	Chrome (Version 87.0.4280)
	+	Edge (Version 87.0.664.5)
	+	Firefox (Version 83.0)
	+	Safari (Version 14.0.1)

80- (or n-) column indicator in diff viewer

Information

Reviewers

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Description:

Commits:

Groups:

Diff:

Added Files:

Checks run (2 succeeded)

Removed Files:

Added Files:

Change Summary:

Added Files:

Change Summary:

Testing Done:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Summary:

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff:

Added Files:

Checks run (2 succeeded)

Change Summary:

Removed Files:

Change Summary:

Description:

Change Summary:

Description: