Summary

Add a new revision selector UI.

Review Request #4695 — Created Oct. 7, 2013 and submitted Oct. 11, 2013, 5:07 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

2252

Depends On

Reviewers

Groups

reviewboard

People

Description

Add a new revision selector UI.

This change gets rid of our old rows of boxes in favor of a new slider widget.
The slider has ticks for "orig" (the original revision) and each diff revision.
By sliding one of the two handles around, the user can select any individual
diff revision, or an interdiff.

There's currently one shortcut, where you can click an individual number to
jump straight to that diff. I've added a pointer cursor here to help make this
discoverable. I'd also like to add the ability to click and drag across two
labels to jump to an interdiff.

The rest of the UI around this (the revision label, comments hint, and others)
all jump around way more than I'd like. I'm currently playing with a few
designs to make it so nothing will move around when the info changes.

Testing Done


Played around a lot with the mouse handling trying to get it to break. The

  "grab" works pretty well.
Selected various revisions and interdiff revisions. Saw that the correct one

  was loaded all the time.
Checked that the UI didn't show up when viewing a single revision.

Files

Issues

Description	From	Last Updated
Can we make a definition of some sort in defs.less for this, to keep vendor shortcuts out of other files? …	chipx86	Oct. 9, 2013, 12:08 a.m.
Same here.	chipx86	Oct. 9, 2013, 12:08 a.m.
What's 30?	chipx86	Oct. 9, 2013, 12:14 a.m.
No need for the 'px' if you do .width() instead.	chipx86	Oct. 9, 2013, 12:11 a.m.
No need for the space before />	chipx86	Oct. 9, 2013, 12:11 a.m.
Same here.	chipx86	Oct. 9, 2013, 12:11 a.m.
Instead of this, you should be able to add this in initialize: _.bindAll(this, '_onHandleMouseUp', '_onHandleMouseDown') And then just reference _onHandleMouseUp …	chipx86	Oct. 9, 2013, 12:13 a.m.
Should be removed I think?	chipx86	Oct. 9, 2013, 12:13 a.m.
Here too.	chipx86	Oct. 9, 2013, 12:13 a.m.
Blank line after var.	chipx86	Oct. 9, 2013, 12:13 a.m.
Trailing comma.	chipx86	Oct. 9, 2013, 12:13 a.m.
This will appear as a CSS rule by default. It needs a ().	chipx86	Oct. 11, 2013, 5:05 p.m.
Here too.	chipx86	Oct. 11, 2013, 5:05 p.m.

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
Ignored Files:
reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
reviewboard/static/rb/css/icons.less
reviewboard/static/rb/css/diffviewer.less
reviewboard/static/rb/images/icons.svg
reviewboard/templates/diffviewer/view_diff.html
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
reviewboard/static/rb/images/icons.png
reviewboard/static/rb/images/icons@2x.png

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
Ignored Files:
reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
reviewboard/static/rb/css/icons.less
reviewboard/static/rb/css/diffviewer.less
reviewboard/static/rb/images/icons.svg
reviewboard/templates/diffviewer/view_diff.html
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
reviewboard/static/rb/images/icons.png
reviewboard/static/rb/images/icons@2x.png

Can you run jshint and make sure it's happy?

reviewboard/static/rb/css/diffviewer.less (Diff revision 1)

The issue has been resolved. Show all issues

Can we make a definition of some sort in defs.less for this, to keep vendor shortcuts out of other files? .grabbing-cursor or something.

reviewboard/static/rb/css/diffviewer.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
What's 30?
```
1. david Oct. 9, 2013, 12:16 a.m.
  It's the spacing for the ticks.
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for the 'px' if you do .width() instead.
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for the space before />
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```

reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)

The issue has been resolved. Show all issues

Instead of this, you should be able to add this in initialize:

_.bindAll(this, '_onHandleMouseUp', '_onHandleMouseDown')

And then just reference _onHandleMouseUp and _onHandleMouseDown without worrying about the context.

reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be removed I think?
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Here too.
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line after var.
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing comma.
```

Diff:

Revision 2 (+333 -143)

Show changes

	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/diffviewer.less
	reviewboard/static/rb/css/icons.less
	reviewboard/static/rb/images/icons.svg
	reviewboard/static/rb/images/icons.png
	reviewboard/static/rb/images/icons@2x.png
	reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
	reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
	2 more

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
Ignored Files:
reviewboard/static/rb/images/icons.png
reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
reviewboard/static/rb/css/icons.less
reviewboard/static/rb/css/diffviewer.less
reviewboard/static/rb/images/icons.svg
reviewboard/templates/diffviewer/view_diff.html
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
reviewboard/static/rb/css/defs.less
reviewboard/static/rb/images/icons@2x.png
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
Ignored Files:
reviewboard/static/rb/images/icons.png
reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
reviewboard/static/rb/css/icons.less
reviewboard/static/rb/css/diffviewer.less
reviewboard/static/rb/images/icons.svg
reviewboard/templates/diffviewer/view_diff.html
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
reviewboard/static/rb/css/defs.less
reviewboard/static/rb/images/icons@2x.png
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Ship it!

Only one thing stands out. Looks good after this.

reviewboard/static/rb/css/defs.less (Diff revision 2)
The issue has been resolved. Show all issues
```
This will appear as a CSS rule by default. It needs a ().
```
reviewboard/static/rb/css/defs.less (Diff revision 2)
The issue has been resolved. Show all issues
```
Here too.
```

Status:: Discarded

Status:: Completed
Change Summary:: Pushed to master (3bb6226).