Summary

[WIP] Added a floating banner to the diff view

Review Request #8746 — Created Feb. 14, 2017 and discarded Sept. 14, 2021, 11:17 p.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 on the page. On this banner, I will further add a
collapsed list of the diff files, showing only the the file that the user is currently
looking at. The user will be able to expand this list to navigate to the other files.

Screenshots and demo video: Notion

Testing Done

I have tested the floating banner on the dev server on review requests with multiple
diff files.

I have started writing jasmine tests for the floating banner. This is still a WIP.
The banner has been manually tested (including responsiveness) on Chrome, Safari, and
Firefox. Click here to see GIFs of the manual tests (running on diff revision 13).

Further testing on other browsers are required.

Issues

Description	From	Last Updated
I think this would be more readable if this else was on a new line.	DA DanielPBak	March 5, 2017, 8:05 a.m.
Same here.	DA DanielPBak	March 5, 2017, 8:05 a.m.
remove blank line	RK rkdhatt	March 5, 2017, 8:03 a.m.
Alphabetize	brennie	March 18, 2017, 9:40 p.m.
Doc comments begin with /**.	brennie	March 18, 2017, 9:40 p.m.
Doc comments begin with /** in JS	brennie	March 18, 2017, 9:40 p.m.
How about viewType	brennie	March 18, 2017, 9:41 p.m.
This can always run, since it is just a call to bind. It doesn't hurt. Alternatively we can add this …	brennie	March 25, 2017, 1:16 p.m.
Instead of using each, we should do a for loop. That way we can break out after finding the element …	brennie	March 25, 2017, 1:16 p.m.
This isnt a jQuery. To get a jQuery (and use the .show() method i suggested) use this._$items.eq(index).	brennie	March 18, 2017, 9:44 p.m.
you can do $bannerTableRow.show() here.	brennie	March 18, 2017, 9:44 p.m.
This should be a class, not an ID.	brennie	March 25, 2017, 1:15 p.m.
border has a shorthand property as following: border: border-width border-style border-color or you can define border-width as following: border-width: top …	AN anni_cao	April 6, 2017, 7:56 p.m.
Why do you need break at the end of if statement? It will jump out of the loop anyway, right?	AN anni_cao	April 2, 2017, 9:44 a.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/views/floatingBannerView.js
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.js
    reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/views/floatingBannerView.js
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffReviewableCollection.js
    reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

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 on the page. On this banner, I will further add a
collapsed list of the diff files, showing only the the file that the user is currently
looking at. The user will be able to expand this list to navigate to the other files.

~
This week I improved the banner styling and further developed the diffFileIndexModel.
~
This week I improved the banner styling and further developed the diffFileIndexModel.
+ See screenshots here:
+ https://www.notion.so/Week-4-Feb-15th-5974361d443744ed9242030cc19908c1
+ I have also written about this week's accomplishments under "Week 4" here:
+ https://www.notion.so/Task-List-Project-Floating-Banner-a03043d62abf4b4b840a8bed4afb78a4

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 on the page. On this banner, I will further add a
collapsed list of the diff files, showing only the the file that the user is currently
looking at. The user will be able to expand this list to navigate to the other files.

~
This week I improved the banner styling and further developed the diffFileIndexModel.
~ See screenshots here:
~ https://www.notion.so/Week-4-Feb-15th-5974361d443744ed9242030cc19908c1
~
This week I improved the banner styling and the diffFileIndexModel.
~ See screenshots of the updated floating banner on this notion page.
~ See this week's accomplishments under "Week 4" on this notion page.
- I have also written about this week's accomplishments under "Week 4" here:
- https://www.notion.so/Task-List-Project-Floating-Banner-a03043d62abf4b4b840a8bed4afb78a4

reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js (Diff revision 1)

The issue has been dropped. Show all issues

I think this would be more readable if this else was on a new line.

Mons Feb. 26, 2017, 9:53 p.m.

I totally agree with you. However, I am following the same coding style / conventions
that are used elsewhere in RB's codebase. The rest of RB's Javascript codebase uses
else statements on the same line as the curly bracket.

reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Same here.
```
1. MO
  
  Mons Feb. 26, 2017, 9:53 p.m.
  See reply above.

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 on the page. On this banner, I will further add a
		collapsed list of the diff files, showing only the the file that the user is currently
		looking at. The user will be able to expand this list to navigate to the other files.
-
-		This week I improved the banner styling and the `diffFileIndexModel`.
-		See screenshots of the updated floating banner on this notion page.
-		See this week's accomplishments under "Week 4" on this notion page.

Commit:

8adb665eedcec9649b2b968f97db937cf3034fba

205feefa2d446667c39055ea416e10fa89fa103b

Diff:

Revision 2 (+119 -24)

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/views/floatingBannerView.js
	reviewboard/templates/diffviewer/view_diff.html

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/models/diffFileIndexModel.js
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/diffviewer/views/floatingBannerIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/static/rb/js/views/floatingBannerView.js
	1 more

~		I have tested the floating banner on the dev server on review requests with only 1
~		diff file. Further testing must be done on review requests with multiple diff files.
	~	I have tested the floating banner on the dev server on review requests with multiple
	~	diff files.

		I have tested the floating banner on the dev server on review requests with multiple
		diff files.
~		So far I have only tested on Chrome. Further testing on other browsers must be done.
	~	The banner has been tested on Chrome and Safari. Further testing on other browsers
	+	must be done.

		I have tested the floating banner on the dev server on review requests with multiple
		diff files.
~		The banner has been tested on Chrome, Safari, and Firefox. Further testing on other
~		browsers is required.
	~	I have started writing jasmine tests for the floating banner. This is still a WIP.
	~	The banner has been manually tested (including responsiveness) on Chrome, Safari, and
	+	Firefox. Further testing on other browsers is required.

[WIP] Added a floating banner to the diff view

Information

Reviewers

Checks run (2 succeeded, 1 failed with error)

Checks run (2 succeeded, 1 failed with error)

Checks run (2 succeeded, 1 failed with error)

Checks run (2 succeeded, 1 failed with error)

Checks run (2 succeeded, 1 failed with error)

Checks run (2 succeeded, 1 failed with error)