Add a new revision selector UI.

Review Request #4695 — Created Oct. 7, 2013 and submitted

david
Review Board
master
2252
reviewboard

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.

  • 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.
Loading file attachments...

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? ...

chipx86chipx86

Same here.

chipx86chipx86

What's 30?

chipx86chipx86

No need for the 'px' if you do .width() instead.

chipx86chipx86

No need for the space before />

chipx86chipx86

Same here.

chipx86chipx86

Instead of this, you should be able to add this in initialize: _.bindAll(this, '_onHandleMouseUp', '_onHandleMouseDown') And then just reference _onHandleMouseUp ...

chipx86chipx86

Should be removed I think?

chipx86chipx86

Here too.

chipx86chipx86

Blank line after var.

chipx86chipx86

Trailing comma.

chipx86chipx86

This will appear as a CSS rule by default. It needs a ().

chipx86chipx86

Here too.

chipx86chipx86
reviewbot
  1. 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

  2. 
      
reviewbot
  1. 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

  2. 
      
chipx86
  1. Can you run jshint and make sure it's happy?

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

    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.

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

    Same here.

    1. It's the spacing for the ticks.

  4. No need for the 'px' if you do .width() instead.

  5. No need for the space before />

  6. 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.

  7. 
      
david
reviewbot
  1. 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

  2. 
      
reviewbot
  1. 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

  2. 
      
chipx86
  1. Only one thing stands out. Looks good after this.

  2. reviewboard/static/rb/css/defs.less (Diff revision 2)
     
     

    This will appear as a CSS rule by default. It needs a ().

  3. reviewboard/static/rb/css/defs.less (Diff revision 2)
     
     

    Here too.

  4. 
      
david
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (3bb6226).

Loading...