• 
      

    Improve the styling and performance of diff expansion in reviews.

    Review Request #9057 — Created July 7, 2017 and submitted

    Information

    Review Board
    release-3.0.x
    68b089b...

    Reviewers

    The existing implementation of diff expansion in reviews used
    JavaScript-side animations for expanding the collapsed expansion
    controls in a diff on hover. As the controls expanded, the content below
    the diff would get pushed down. This caused a lot of rendering on the
    page, which could be slow and made scrolling through the review request
    page a bit more tedious.

    The new implementation uses CSS transforms and transitions to shift and
    scale the controls and the diff header, which allows us to expand
    outward from the diff without impacting the position of the diff itself
    or any of the content below it on the page. This is much faster,
    resulting in a smoother animation and less jumpiness on the page.

    To do this, we begin the diff fragments in a collapsed state, scaling
    down the controls and shifting the diff header down by the same amount.
    We do this without transitions. After it's been loaded and scaled down,
    transitions are enabled, which will then start taking place when the
    user hovers.

    Tested on Chrome, Firefox, and IE9+.

    Tested with transitions fully disabled to make sure functionality
    still worked.

    Tested with diffs containing headers just above, just below, both
    above and below, and neither above nor below.


    Description From Last Updated

    /**

    daviddavid

    Can be _showControls(diffEls) {

    daviddavid

    /**

    daviddavid

    Can be _hideControls(diffEls) {

    daviddavid

    Don't need the ()s around the variable name when there's only one arg.

    daviddavid

    /** Mind also fixing up the format of this comment to be summary/description?

    daviddavid

    This can be one-lined: _.defer(() => $container.addClass('allow-transitions'));

    daviddavid

    /**

    daviddavid

    _tryShowControlsDelayed(diffEls)

    daviddavid

    /**

    daviddavid

    _tryHideControlsDelayed(diffEls)

    daviddavid
    david
    1. Woo! This is awesome.

      I have just a few formatting comments for the JS.

    2. Show all issues

      /**

    3. Show all issues

      Can be _showControls(diffEls) {

    4. Show all issues

      /**

    5. Show all issues

      Can be _hideControls(diffEls) {

    6. Show all issues

      Don't need the ()s around the variable name when there's only one arg.

      1. Ah, yeah. I'm in the habit of putting it since the single argument version is the only version that doesn't require parens, and I like the consistency. (Been seeing parens used for single arguments in some of the code I've recently been using in a side project as well -- really wish ES6 required them when doing { .. }, honestly.)

    7. Show all issues

      /**

      Mind also fixing up the format of this comment to be summary/description?

    8. Show all issues

      This can be one-lined:

      _.defer(() => $container.addClass('allow-transitions'));
      
    9. Show all issues

      /**

    10. Show all issues

      _tryShowControlsDelayed(diffEls)

    11. Show all issues

      /**

    12. Show all issues

      _tryHideControlsDelayed(diffEls)

    13. 
        
    chipx86
    brennie
    1. Are you a wizard

    2. 
        
    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (bb3f23d)