Summary

Scroll oversized images in the image review UI

Review Request #8377 — Created Sept. 2, 2016 and submitted Sept. 19, 2016, 4:13 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

~~8400~~

Blocks

8392

Commit

2300cf1...

Reviewers

Groups

reviewboard

People

Description

Previously, oversized images in the image review UI would cause them to
overflow outside of the review box and increase the width of the page.
Now, these images are confined to the width of the review box and will
scroll horizontally when they are too large.

Testing Done


Created a review comment on an oversized image; it scrolled with the

  page.
Scrolled an oversized image and created a commment; it worked

  correctly and scrolled with the page.
Ensured the scroll bar only showed when images were oversized for the

  currently selected scale.

Issues

Description	From	Last Updated
Can you test scrolling and then adding a comment?	david	Sept. 16, 2016, 11:03 p.m.
Should be in alphabetical order within the other classes.	chipx86	Sept. 13, 2016, 9:25 a.m.
Since you're adding a new one, can you add docs to the doc comment? ("Model Attributes" section.)	chipx86	Sept. 13, 2016, 9:27 a.m.
Two blank lines.	chipx86	Sept. 13, 2016, 9:28 a.m.
Should be able to cleanly fit all these like: this.listenTo(this.model, 'change:scale', ...)	chipx86	Sept. 13, 2016, 9:28 a.m.
Should be one statement, like before, since we're not chaining.	chipx86	Sept. 13, 2016, 9:28 a.m.
This can be one statement.	chipx86	Sept. 13, 2016, 9:29 a.m.
I really want to avoid using these. I know it's not the most popular opinion, but it ends up resulting …	chipx86	Sept. 13, 2016, 9:41 a.m.
I also want to avoid using for of. This feels like a slim little useful statement, but balloons into a …	chipx86	Sept. 13, 2016, 9:41 a.m.
Same as above. The spacing on either side of the text inside the <li> will be preserved, and we should …	chipx86	Sept. 13, 2016, 9:37 a.m.
No need for the blank line here.	chipx86	Sept. 13, 2016, 9:39 a.m.
This is left over.	chipx86	Sept. 13, 2016, 9:39 a.m.
Trailing newline.	chipx86	Sept. 14, 2016, 1:12 p.m.
We're doing a lot of mixing of local variable declarations and other code. Mind cleaning this up while here by …	chipx86	Sept. 14, 2016, 1:12 p.m.
Might as well add a trailing comma here too.	chipx86	Sept. 14, 2016, 1:13 p.m.
Let's add a trailing comma here, too, while we're at it.	chipx86	Sept. 14, 2016, 1:13 p.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/views/imageReviewableView.es6.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/views/imageReviewableView.es6.js

I'd really like to see testing of commenting while scrolled and the display of the comment block outlines at various zoom/scroll combinations.

Summary:

-	Create a wrapper to scroll oversized images in the image review UI
+	Scroll oversized images in the image review UI

Description:

~		Previously, the image review UI would overflow horizontally and increase
~		the width of the page. Now, the review UI is limited to the file
~		attachment box and it will scroll horiztonally within it.
	~	Previously, oversized images in the image review UI would cause them to
	~	overflow outside of the review box and increase the width of the page.
	~	Now, these images are confined to the width of the review box and will
	+	scroll horizontally when they are too large.
	+
	+	Additionally, image scaling has been moved into the `RB.ImageReviewable`
	+	(which has been converted to ES6 to allow for trailing commas) so that
	+	we can listen in the different image views for when the scale changes,
	+	as well as programmatically trigger scale changes without resorting to
	+	invoking a click handler.

Testing Done:

~		Tested the review UI with an oversized image. It scrolled.
	~	Created a review comment on an oversized image; it scrolled with the page.
	+	Ensured the scroll bar only showed when images were oversized for the currently selected scale.

Diff:

Revision 2 (+127 -99)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/image-review-ui.less
	reviewboard/static/rb/js/models/imageReviewableModel.js
	reviewboard/static/rb/js/views/imageReviewableView.es6.js
	reviewboard/static/rb/js/views/regionCommentBlockView.es6.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/views/regionCommentBlockView.es6.js
    reviewboard/static/rb/js/views/imageReviewableView.es6.js
    reviewboard/static/rb/js/models/imageReviewableModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/views/regionCommentBlockView.es6.js
    reviewboard/static/rb/js/views/imageReviewableView.es6.js
    reviewboard/static/rb/js/models/imageReviewableModel.es6.js

reviewboard/static/rb/css/pages/image-review-ui.less (Diff revision 2)
Show all issues
```
Should be in alphabetical order within the other classes.
```

reviewboard/static/rb/js/models/imageReviewableModel.es6.js (Diff revision 2)

Show all issues

Since you're adding a new one, can you add docs to the doc comment? ("Model Attributes" section.)

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 2)
Show all issues
```
Two blank lines.
```

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 2)

Show all issues

Should be able to cleanly fit all these like:

this.listenTo(this.model,
              'change:scale',
              ...)

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 2)
Show all issues
```
Should be one statement, like before, since we're not chaining.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 2)
Show all issues
```
This can be one statement.
```

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 2)

Show all issues

I really want to avoid using these. I know it's not the most popular opinion, but it ends up resulting in much larger strings, as we have no real control over where spaces and newlines live. All leading indentation ends up as part of the string. This cannot be minified out, resulting in larger JavaScript, and it's more content for the template parser to parse.

Same below.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 2)

Show all issues

I also want to avoid using for of. This feels like a slim little useful statement, but balloons into a whole lot of very slow code that cannot be easily optimized out.
This becomes:

var _iteratorNormalCompletion = true;
var _didIteratorError = false;
var _iteratorError = undefined;

try {
    for (var _iterator = scalingFactors[Symbol.iterator](), _step; !(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) {
        var _step$value = _slicedToArray(_step.value, 2);

        var scale = _step$value[0];
        var text = _step$value[1];

        $menu.append(...)

    }
} catch (err) {
    _didIteratorError = true;
    _iteratorError = err;
} finally {
    try {
        if (!_iteratorNormalCompletion && _iterator.return) {
            _iterator.return();
        }
    } finally {
        if (_didIteratorError) {
            throw _iteratorError;
        }
    }
}

chipx86 Sept. 13, 2016, 12:52 a.m.

Hmm, this is a Map, not a plain object, so we're limited here to this or .forEach(), which means a function call per iteration, which isn't as fast as a standard for loop either.
Though, we have a function call anyway, because of iterator.next() and _slicedToArray. So maybe .forEach() is best here. Plus, it'd avoid the exceptions, which can impact optimization in some browsers (though I believe this is less of a problem in modern ones).
(If only it could optimistically do a native for of...)
From http://www.incaseofstairs.com/2015/06/es6-feature-performance/:


for..of is universally slower, ranging from 3 to 20x slower for array iteration over classical array iteration. When iterating over an object with a custom iterator, the performance is also much slower than for..in iteration with hasOwnProperty checks.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 2)

Show all issues

Same as above. The spacing on either side of the text inside the <li> will be preserved, and we should avoid that.

We should also be treating the text as unsafe HTML, in case we need to localize any of it down the road. (I could see options like "Fit to window".) Building as a jQuery is going to be safer here.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 2)
Show all issues
```
No need for the blank line here.
```
reviewboard/static/rb/js/views/regionCommentBlockView.es6.js (Diff revision 2)
Show all issues
```
This is left over.
```

Change Summary:

Addressed Christian's issues.

Diff:

Revision 3 (+137 -103)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/image-review-ui.less
	reviewboard/static/rb/js/models/imageReviewableModel.js
	reviewboard/static/rb/js/models/imageReviewableModel.es6.js
	reviewboard/static/rb/js/views/imageReviewableView.es6.js
	reviewboard/static/rb/js/views/regionCommentBlockView.es6.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/models/imageReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.es6.js
    reviewboard/static/rb/js/models/imageReviewableModel.es6.js
    reviewboard/static/rb/js/views/regionCommentBlockView.es6.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/models/imageReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.es6.js
    reviewboard/static/rb/js/models/imageReviewableModel.es6.js
    reviewboard/static/rb/js/views/regionCommentBlockView.es6.js

Looks pretty good. A couple of small things and then I'm happy (and super looking forward to having this in!)

reviewboard/static/rb/js/models/imageReviewableModel.es6.js (Diff revision 3)
Show all issues
```
Trailing newline.
```

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 3)

Show all issues

We're doing a lot of mixing of local variable declarations and other code. Mind cleaning this up while here by moving the this._max* into its own grouping after the consts?

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 3)
Show all issues
```
Might as well add a trailing comma here too.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 3)
Show all issues
```
Let's add a trailing comma here, too, while we're at it.
```

Change Summary:

Address Christian's issues

Diff:

Revision 4 (+137 -104)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/image-review-ui.less
	reviewboard/static/rb/js/models/imageReviewableModel.js
	reviewboard/static/rb/js/models/imageReviewableModel.es6.js
	reviewboard/static/rb/js/views/imageReviewableView.es6.js
	reviewboard/static/rb/js/views/regionCommentBlockView.es6.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/models/imageReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.es6.js
    reviewboard/static/rb/js/models/imageReviewableModel.es6.js
    reviewboard/static/rb/js/views/regionCommentBlockView.es6.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/models/imageReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.es6.js
    reviewboard/static/rb/js/models/imageReviewableModel.es6.js
    reviewboard/static/rb/js/views/regionCommentBlockView.es6.js

This should really be broken up into two (or three) changes. Only two lines in one file relate to scrolling, and the rest is all the model attribute change and style cleanup.

brennie Sept. 16, 2016, 10:41 p.m.
```
Will do.
```

Show all issues

Can you test scrolling and then adding a comment?

Description:

		Previously, oversized images in the image review UI would cause them to
		overflow outside of the review box and increase the width of the page.
		Now, these images are confined to the width of the review box and will
		scroll horizontally when they are too large.
-
-		Additionally, image scaling has been moved into the `RB.ImageReviewable`
-		(which has been converted to ES6 to allow for trailing commas) so that
-		we can listen in the different image views for when the scale changes,
-		as well as programmatically trigger scale changes without resorting to
-		invoking a click handler.

Testing Done:

		Created a review comment on an oversized image; it scrolled with the page.
~		Ensured the scroll bar only showed when images were oversized for the currently selected scale.
	~	Scrolled an oversized image and created a commment; it worked correctly and scrolled with the page.
	+	Ensured the scroll bar only showed when images were oversized for the currently selected scale.

Depends On:

+	~~8400 - Move image scale into the ImageReviewableModel~~

Diff:

Revision 5 (+15 -3)

Show changes

	reviewboard/static/rb/css/pages/image-review-ui.less
	reviewboard/static/rb/js/views/imageReviewableView.es6.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/views/imageReviewableView.es6.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/views/imageReviewableView.es6.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (eb14738)