Summary

Mobile Diff Viewer (Cosmetic Changes)

Review Request #9396 — Created Nov. 25, 2017 and discarded Feb. 10, 2019, 9:16 a.m.

Information

Owner

tbrockma

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description


Removed review request information from the diff section (leaving it in the reviews section)
Made diff index anchor list collapsable
Fixed the display of the revision table file list on mobile
Changed the styling of the view control buttons on mobile
Changed the styling of the pending review banner on mobile

Testing Done

Added Javascript tests to ensure functionality of collapsing anchor list,
visual inspection and manual-testing of other changes.

Files

Issues

Description	From	Last Updated
This banner still needs to be fixed, but is not included in this review request.	TB tbrockma	Dec. 10, 2017, 7:55 p.m.
E502 the backslash is redundant between brackets	reviewbot	Nov. 25, 2017, 10:12 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Nov. 25, 2017, 10:12 p.m.
Col: 33 Expected '{' and instead saw 'this'.	reviewbot	Nov. 25, 2017, 10:38 p.m.
Col: 33 Expected '{' and instead saw 'this'.	reviewbot	Nov. 25, 2017, 10:12 p.m.
Col: 33 Expected '{' and instead saw 'this'.	reviewbot	Nov. 25, 2017, 10:12 p.m.
This line should be less than 78 characters	MU mukhtar	Dec. 12, 2017, 4:50 p.m.
This line should be less than 78 characters	MU mukhtar	Dec. 3, 2017, 12:49 a.m.
This line should be less than 78 characters	MU mukhtar	Dec. 3, 2017, 12:50 a.m.
This line should be less than 78 characters	MU mukhtar	Dec. 3, 2017, 12:50 a.m.
Col: 23 Expected '===' and instead saw '=='.	reviewbot	Dec. 7, 2017, 11:33 p.m.
Should be single quotes	TB tbrockma	Dec. 9, 2017, 2:42 p.m.
Single quotes.	TB tbrockma	Dec. 9, 2017, 2:42 p.m.
These changes should probably just be in a separate review request with a test making sure the touch screen events …	TB tbrockma	Dec. 9, 2017, 2:42 p.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E502 the backslash is redundant between brackets
```
reviewboard/reviews/tests/test_review_request.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E128 continuation line under-indented for visual indent
```

JSHint

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 33
 Expected '{' and instead saw 'this'.
```

Change Summary:

Removing unnecessarily tracked files.

Summary:

Fix #4492. Deprecated "review_request.get_close_description" and extended with "review_request.close_info", which returns a dictionary containing the values previously returned by "get_close_description", in addition to a datetime object representing the time the review request was closed.

Mobile Diff Viewer Cosmetic Changes

Description:

~		Fixing indent level to please ReviewBot, also fixing previously added undefined reference.
	~	Just some pretty things.
-
-
-
-		Rename closed date view variables, changed additional variable creation into direct dictionary accesses.
-
-
-
-		before plane
-
-
-
-		Merge branch 'master' of https://github.com/reviewboard/reviewboard
-
-
-
-		Moving cosmetics to a separate feature branch.
-
-
-
-		Moved less variable definitions to where they belong.
-
-
-
-		working on cosmetic changes, commit before merge
-
-
-
-		Merge branch 'master' of https://github.com/reviewboard/reviewboard into diffViewerCosmetics
-
-
-
-		test commit

Commit:

b1555f037a246eca38f3b4fb75d0b53e935fd34b

acdae096df838bf7e2438d348d21e92aaf4f6f02

Diff:

Revision 2 (+332 -38)

Show changes

	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/templates/diffviewer/view_diff.html

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 33
 Expected '{' and instead saw 'this'.
```

Commit:

acdae096df838bf7e2438d348d21e92aaf4f6f02

cd6572b2e1d27c6e2aa521086f2a759aea8de398

Diff:

Revision 3 (+329 -38)

Show changes

	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/templates/diffviewer/view_diff.html

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 33
 Expected '{' and instead saw 'this'.
```

Commit:

cd6572b2e1d27c6e2aa521086f2a759aea8de398

987432e34d11743a09a80d42f6339282d8c7a63d

Diff:

Revision 4 (+339 -38)

Show changes

	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/templates/diffviewer/view_diff.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:: ~
Just some pretty things.
~
Removed review request information from the diff section (leaving it in the reviews section)
+
+
+
+
Made the diff index anchor list collapsable
+
+
+
+
Fixed the display of the revision table file list on mobile
+
+
+
+
Changed the styling of the view control buttons on mobile
+
+
+
+
Changed the styling of the pending review banner on mobile
+
+
Testing Done:: +
Added Javascript tests to ensure functionality of collapsing anchor list,
+ visual inspection and manual-testing of other changes.

Summary:: Mobile Diff Viewer Cosmetic Changes
[WIP] Mobile Diff Viewer Cosmetic Changes

reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This line should be less than 78 characters
```
reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This line should be less than 78 characters
```
reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This line should be less than 78 characters
```
reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This line should be less than 78 characters
```

Description:: ~
Removed review request information from the diff section (leaving it in the reviews section)
~
~
~
~
Made the diff index anchor list collapsable
~
Removed review request information from the diff section (leaving it in the reviews section)
~
Made diff index anchor list collapsable
~
Fixed the display of the revision table file list on mobile
~
Changed the styling of the view control buttons on mobile
~
Changed the styling of the pending review banner on mobile
-
-
-
-
Fixed the display of the revision table file list on mobile
-
-
-
-
Changed the styling of the view control buttons on mobile
-
-
-
-
Changed the styling of the pending review banner on mobile
-
-
Added Files:

```
Cosmetic curiosity
```

For change 5 why did you go with blue instead of the standard green?

tbrockma Nov. 30, 2017, 5:29 p.m.

You could make arguments for different colors based on the context of the banner. Am I being warned that I have a pending review that requires immediate action (yellow/red)? Is it a successful operation that requires no input from me (green)? Is it non-urgent information (blue)?

But to be honest, it was a pretty arbitrary decision. I tried it with the original green but I felt it clashed with the line addition color scheme of the diffs (because they're slightly different greens). Changing the banner color to the line addition green didn't look that great either.

I went with blue, because I liked the blue. If others feel differently I am more than okay to change it to whatever.

Summary:: [WIP] Mobile Diff Viewer Cosmetic Changes
[WIP] Mobile Diff Viewer (Cosmetic Changes)

Commit:

987432e34d11743a09a80d42f6339282d8c7a63d

3a1ff08f7e3dc8b0520f0fd0b75f057e532c1639

Diff:

Revision 5 (+334 -38)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/templates/diffviewer/view_diff.html

Removed Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

3a1ff08f7e3dc8b0520f0fd0b75f057e532c1639

f13b51b613d8077b855d2222f1991e3899f39919

Diff:

Revision 6 (+334 -38)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/templates/diffviewer/view_diff.html

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		Removed review request information from the diff section (leaving it in the reviews section)
		Made diff index anchor list collapsable
		Fixed the display of the revision table file list on mobile
		Changed the styling of the view control buttons on mobile
		Changed the styling of the pending review banner on mobile
	+	Fixed interactions with the revision selector on touchscreens

Commit:

f13b51b613d8077b855d2222f1991e3899f39919

5133c2f8c9ab867527dacea22840633cde87930f

Diff:

Revision 7 (+367 -41)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/static/rb/js/views/revisionSelectorView.js
	reviewboard/templates/diffviewer/view_diff.html

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/revisionSelectorView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 23
 Expected '===' and instead saw '=='.
```

Commit:

5133c2f8c9ab867527dacea22840633cde87930f

f3afcb252d517fba880ff6f3bc639436c0d58523

Diff:

Revision 8 (+367 -41)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/static/rb/js/views/revisionSelectorView.js
	reviewboard/templates/diffviewer/view_diff.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Should be single quotes
```
reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Single quotes.
```

reviewboard/static/rb/js/views/revisionSelectorView.js (Diff revision 8)

The issue has been resolved. Show all issues

These changes should probably just be in a separate review request with a test making sure the touch screen events are handled correctly

Commit:

f3afcb252d517fba880ff6f3bc639436c0d58523

492c71060b3b915c4bc5e59679c7c2d0010dd28e

Diff:

Revision 9 (+339 -38)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/templates/diffviewer/view_diff.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:: Removed review request information from the diff section (leaving it in the reviews section)

Made diff index anchor list collapsable

Fixed the display of the revision table file list on mobile

Changed the styling of the view control buttons on mobile

Changed the styling of the pending review banner on mobile
-
Fixed interactions with the revision selector on touchscreens

Summary:: [WIP] Mobile Diff Viewer (Cosmetic Changes)
Mobile Diff Viewer (Cosmetic Changes)

Commit:

492c71060b3b915c4bc5e59679c7c2d0010dd28e

35327170cb5514c64b1ef2da8b156dd9c507bb23

Diff:

Revision 10 (+345 -38)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/templates/diffviewer/view_diff.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

An issue was opened. Show all issues

Publish banner

This banner still needs to be fixed, but is not included in this review request.

Status:: Discarded

~		Just some pretty things.
	~	Removed review request information from the diff section (leaving it in the reviews section)
	+
	+
	+
	+	Made the diff index anchor list collapsable
	+
	+
	+
	+	Fixed the display of the revision table file list on mobile
	+
	+
	+
	+	Changed the styling of the view control buttons on mobile
	+
	+
	+
	+	Changed the styling of the pending review banner on mobile
	+
	+

	+	Added Javascript tests to ensure functionality of collapsing anchor list,
	+	visual inspection and manual-testing of other changes.