Summary

Add a Diff Viewer view for only the new file

Review Request #11226 — Created Oct. 16, 2020 and updated Nov. 20, 2020, 2:26 a.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 shows both the old and new view for every diff. 

Sometimes, we want to just look at the new view and not the whole diff with 

the existing code. Adding the feature to hide the old view and only show the 

new view will allow users to have an uncluttered view.
We added a [Show/Hide] toggle to the revision-row on the diff viewer. This

button will toggle between showing both views and only showing the new view. 

The button toggles a show-new-only class which show or hide the old view with 

the CSS display: none property. If the line contain comments, the comment 

flags are detached from the old view's column and appended to the new view's

column using jQuery's detach() and appendTo() function, vice versa.

Testing Done

Testing Done:
Wrote onShowNewClicked and On show new only in
diffReviewableViewTests.es6.js.

Tested following behaviour in browser:

Added comments before toggling, able to view in both views
Added comments after toggling, able to view in both views

Commits

Summary	ID
The current diff viewer shows both the old and new view for every diff. Sometimes, we want to just look at the new view and not the whole diff with the existing code. Adding the feature to hide the old view and only show the new view will allow users to have an uncluttered view. We added a `[Show/Hide]` toggle to the `revision-row` on the diff viewer. This button will toggle between showing both views and only showing the new view. The button toggles a `show-new-only` class which show or hide the old view with the CSS `display: none` property. If the line contain comments, the comment flags are detached from the old view's column and appended to the new view's column using jQuery's `detach()` and `appendTo()` function, vice versa. Testing Done: Wrote `onShowNewClicked` and `On show new only` in `diffReviewableViewTests.es6.js`. Tested following behaviour in browser: * Added comments before toggling, able to view in both views * Added comments after toggling, able to view in both views	6ac04ab5ec2f58991b8a7a4587d3796c2175c243

Summary

The current diff viewer shows both the old and new view for every diff.

Sometimes, we want to just look at the new view and not the whole diff with the existing code. Adding the feature to hide the old view and only show the new view will allow users to have an uncluttered view. We added a `[Show/Hide]` toggle to the `revision-row` on the diff viewer. This button will toggle between showing both views and only showing the new view. The button toggles a `show-new-only` class which show or hide the old view with the CSS `display: none` property. If the line contain comments, the comment flags are detached from the old view's column and appended to the new view's column using jQuery's `detach()` and `appendTo()` function, vice versa. Testing Done: Wrote `onShowNewClicked` and `On show new only` in `diffReviewableViewTests.es6.js`. Tested following behaviour in browser: * Added comments before toggling, able to view in both views * Added comments after toggling, able to view in both views

6ac04ab5ec2f58991b8a7a4587d3796c2175c243

Files

flake8 passed.

JSHint passed.

Change Summary:

Updated review request and added UI screenshots.

Description:

~		Update Show New function to toggle.
	~	The current diff viewer shows both the old and new view for every diff.
	+	Sometimes, we want to just look at the new view and not the whole diff with
	+	the existing code. Adding the feature to hide the old view and only show the
	+	new view will allow users to have an uncluttered view.
	+
	+	We added a `[Show/Hide]` toggle to the `revision-row` on the diff viewer. This
	+	button will toggle between showing both views and only showing the new view.
	+	The button toggles a `show-new-only` class which show or hide the old view with
	+	the CSS `display: none` property. If the line contain comments, the comment
	+	flags are detached from the old view's column and appended to the new view's
	+	column using jQuery's `detach()` and `appendTo()` function, vice versa.

Testing Done:

	+	Testing Done:
	+
	+	Tested following behaviour in browser:
	+
	+	Added comments before toggling, able to view in both views
	+	Added comments after toggling, able to view in both views

Commits:

	Summary	ID
	Add button to show new only.	7cc388330d712a50d985451edc64805db8599ec3
	Update Show New function to toggle.	57a3ec9619b329fe9b28d72fb4209d92fc81c971
	The current diff viewer shows both the old and new view for every diff. Sometimes, we want to just look at the new view and not the whole diff with the existing code. Adding the feature to hide the old view and only show the new view will allow users to have an uncluttered view. We added a `[Show/Hide]` toggle to the `revision-row` on the diff viewer. This button will toggle between showing both views and only showing the new view. The button toggles a `show-new-only` class which show or hide the old view with the CSS `display: none` property. If the line contain comments, the comment flags are detached from the old view's column and appended to the new view's column using jQuery's `detach()` and `appendTo()` function, vice versa. Testing Done: Tested following behaviour in browser: * Added comments before toggling, able to view in both views * Added comments after toggling, able to view in both views	50b01e1fb6d3dd20240602823c2ded37844becc3

Diff:

Revision 2 (+94)

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:

Add JS unit tests.

Testing Done:

~		Testing Done:
	~	Testing Done:
	+	Wrote `onShowNewClicked` and `On show new only` in
	+	`diffReviewableViewTests.es6.js`.
	+
	+

		Tested following behaviour in browser:

		Added comments before toggling, able to view in both views
		Added comments after toggling, able to view in both views

Commits:

	Summary	ID
	The current diff viewer shows both the old and new view for every diff. Sometimes, we want to just look at the new view and not the whole diff with the existing code. Adding the feature to hide the old view and only show the new view will allow users to have an uncluttered view. We added a `[Show/Hide]` toggle to the `revision-row` on the diff viewer. This button will toggle between showing both views and only showing the new view. The button toggles a `show-new-only` class which show or hide the old view with the CSS `display: none` property. If the line contain comments, the comment flags are detached from the old view's column and appended to the new view's column using jQuery's `detach()` and `appendTo()` function, vice versa. Testing Done: Tested following behaviour in browser: * Added comments before toggling, able to view in both views * Added comments after toggling, able to view in both views	50b01e1fb6d3dd20240602823c2ded37844becc3
	The current diff viewer shows both the old and new view for every diff. Sometimes, we want to just look at the new view and not the whole diff with the existing code. Adding the feature to hide the old view and only show the new view will allow users to have an uncluttered view. We added a `[Show/Hide]` toggle to the `revision-row` on the diff viewer. This button will toggle between showing both views and only showing the new view. The button toggles a `show-new-only` class which show or hide the old view with the CSS `display: none` property. If the line contain comments, the comment flags are detached from the old view's column and appended to the new view's column using jQuery's `detach()` and `appendTo()` function, vice versa. Testing Done: Wrote `onShowNewClicked` and `On show new only` in `diffReviewableViewTests.es6.js`. Tested following behaviour in browser: * Added comments before toggling, able to view in both views * Added comments after toggling, able to view in both views	f414482469c0797f1e5b9475ea5fa84240c07bed

Diff:

Revision 3 (+196 -2)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix tests in diffReviewableViewTests.es6.js

Summary:

[WIP] Add a Diff Viewer view for only the new file

Add a Diff Viewer view for only the new file

Commits:

	Summary	ID
	The current diff viewer shows both the old and new view for every diff. Sometimes, we want to just look at the new view and not the whole diff with the existing code. Adding the feature to hide the old view and only show the new view will allow users to have an uncluttered view. We added a `[Show/Hide]` toggle to the `revision-row` on the diff viewer. This button will toggle between showing both views and only showing the new view. The button toggles a `show-new-only` class which show or hide the old view with the CSS `display: none` property. If the line contain comments, the comment flags are detached from the old view's column and appended to the new view's column using jQuery's `detach()` and `appendTo()` function, vice versa. Testing Done: Wrote `onShowNewClicked` and `On show new only` in `diffReviewableViewTests.es6.js`. Tested following behaviour in browser: * Added comments before toggling, able to view in both views * Added comments after toggling, able to view in both views	f414482469c0797f1e5b9475ea5fa84240c07bed
	The current diff viewer shows both the old and new view for every diff. Sometimes, we want to just look at the new view and not the whole diff with the existing code. Adding the feature to hide the old view and only show the new view will allow users to have an uncluttered view. We added a `[Show/Hide]` toggle to the `revision-row` on the diff viewer. This button will toggle between showing both views and only showing the new view. The button toggles a `show-new-only` class which show or hide the old view with the CSS `display: none` property. If the line contain comments, the comment flags are detached from the old view's column and appended to the new view's column using jQuery's `detach()` and `appendTo()` function, vice versa. Testing Done: Wrote `onShowNewClicked` and `On show new only` in `diffReviewableViewTests.es6.js`. Tested following behaviour in browser: * Added comments before toggling, able to view in both views * Added comments after toggling, able to view in both views	6ac04ab5ec2f58991b8a7a4587d3796c2175c243

Diff:

Revision 4 (+208 -2)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

I pulled down the patch and it works really nicely!
I cannot comment much on the JQuery, since I do not have much experience using it, but the code looks clean.
Though I think this is rather a small detail, I thought I would mention it anyway. It would be cool if the [hide]/[show] buttons were instead like an eye that is crossed out when it cannot be seen. 
Sort of like when using 'eye' and 'eye-crossed' from the font-awesome library. I know that Review Board has its own icons for download 'rb-icon' and I am unsure whether there are any icons for an eye. 
Again, this is purely a cosmetic change and I am not sure how the mentors would feel about this sort of change. But I think it would be more consistent with the already existing download button.

keanweng Nov. 20, 2020, 1:15 a.m.

Thanks for testing it out, Jacob! I really like the suggestion of adding an icon as it matches better with the rest of the UI.
I've disccused with the mentors previously and the [Show/Hide] is meant to be temporary to test out functionality. They'll have to rethink some of this later on.

I downloaded the diff file and it worked perfectly on my machine! I also ran the javascript tests and everything passes on my machine as well.