Summary

[WIP] Added a floating banner to the diff view

Review Request #8714 — Created Feb. 4, 2017 and discarded Feb. 16, 2017, 10:32 a.m.

Information

Owner

Mons

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

We currently have to scroll all the way to the top if we want to be navigated to a
specific diff file.

I added a floating banner to the diff view that sticks to the top of the page when
the user has scrolled down. I added text to the banner: "Navigate: ", as well as
a dropdown button. This button is a list of all diff file names. The current
state displays the first diff filename on default, and each diff filename
when hovering over the arrow. The default filename is duplicated at the
moment. I also added scrolling functionality when the button gets clicked.
This will take the user's view to the anchor tag of the selected diff file.

Screenshots of the current state of my project:
https://www.notion.so/Week-2-Feb-4th-6eb70ca4cd8e4c4583bbf4d439533e31

Testing Done

I have tested the floating banner on the testing server using dummy diff files
created by RB Tools.

Issues

Description	From	Last Updated
Correct me if I'm wrong, but: I don't understand why you're attaching the diff-banner to the template meant for each …	szhang	Feb. 10, 2017, 10:45 a.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/views/floatingBannerView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/views/floatingBannerView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Description:: We currently have to scroll all the way to the top if we want to be navigated to a
specific diff file.

I added a floating banner to the diff view that sticks to the top of the page when
the user has scrolled down. I added text to the banner: "Navigate: ", as well as
a dropdown button. This button is a list of all diff file names. The current
state displays the first diff filename on default, and each diff filename
when hovering over the arrow. The default filename is duplicated at the
moment. I also added scrolling functionality when the button gets clicked.
This will take the user's view to the anchor tag of the selected diff file.
+
+
Screenshots of the current state of my project:
+ https://www.notion.so/Week-2-Feb-4th-6eb70ca4cd8e4c4583bbf4d439533e31

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1)

The issue has been resolved. Show all issues

Correct me if I'm wrong, but: I don't understand why you're attaching the diff-banner to the template meant for each file's diff in the diff page.
On line 266, $diffs is assigned to the empty div with the id #diffs:
$diffs = $('#diffs').empty();

Then on line 270, a function begins that executes for each file:
files.each(function(file) {

In this function, the line adding the template is here (line 275):
$diffs.append(this._fileEntryTemplate(file.attributes));

So it appears that the code adds the template multiple times to the div with the id #diffs.
I don't think you want to make the banner appear multiple times, just once. Perhaps there is a better place to add the banner, like reviewboard/templates/diffviewer/view_diff.html?

Mons Feb. 10, 2017, 10:50 a.m.

Thanks a lot for pointing out this bug / bad practice, Simon!

I didn't get the chance to test the banner on review
requests containing multiple diff files, and I can
only imagine what it would look like if I had 10+
diff files in the diff view.

Your explanation makes total sense. My intentions were not
to add it once for each diff file, so this was an awesome
catch. I removed the divs from the diffViewerPageView and
added them to view_diff.html.

The banner only gets added once now.

Mons Feb. 10, 2017, 10:55 a.m.

Let me correct myself: My next [WIP] review request will include the update!

Status:: Discarded
Change Summary:: multiple review requests for the same changes.

		We currently have to scroll all the way to the top if we want to be navigated to a
		specific diff file.

		I added a floating banner to the diff view that sticks to the top of the page when
		the user has scrolled down. I added text to the banner: "Navigate: ", as well as
		a dropdown button. This button is a list of all diff file names. The current
		state displays the first diff filename on default, and each diff filename
		when hovering over the arrow. The default filename is duplicated at the
		moment. I also added scrolling functionality when the button gets clicked.
		This will take the user's view to the anchor tag of the selected diff file.
	+
	+	Screenshots of the current state of my project:
	+	https://www.notion.so/Week-2-Feb-4th-6eb70ca4cd8e4c4583bbf4d439533e31