Summary

Code Dump: High DPI Image Review

Review Request #7754 — Created Nov. 4, 2015 and discarded June 3, 2016, 5:24 p.m.

Information

Owner

chronicleyu

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

brennie, chipx86, david, mike_conley

Description

Code Dump: High DPI Image Review
11/04/2015
WIP of the drop down menu for selecting image scale. 

(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)

11/07/2015
Resolved menu positioning problem (Problem # 1)

Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)
(RESOLVED - 11/07/2015) Problem # 2 - I put a console.log(this.$el); into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.

11/13/2015
Re-formatted menu html code to be more readable. 

Added event binding to the menu options. 

Hard coded the event's function to increase image size by 200%.
 NOTE: The week's goal was to try and understand the RB's implementation of Backbone.js and jQuery, and to try and get any kind of change in size of the image. So for now, clicking on the menu options will keep increasing the image size by 200%. Will begin working on the actual implementation of the buttons starting this week. 

11/15/2015
Full menu scaling capabilities added for ImageAttachmentView.

Minor rearrangement of code.

Change menu text color from blue to black.

11/21/2015
WIP on getting scaling to work on OnionDiff mode. It is proving to be more challenging than imageAttachmentView mode because it looks like I'll have to change the CSS of multiple classes rather than just one.
 Problem # 3 - When there are diffs, I can't get the menu to appear in line with the image file names. I also tried putting the menu in the bar with the mode selections but that is giving me positional problems too. 
Moved var statement for $headerInner to top of function.

Standardized the formatting of function documentation.

Removal of unnecessary lines and statements.

11/28/2015
Full menu scaling capabilities added for ImageTwoUpDiffView and ImageOnionDiffView.

Moved var statement for $headerInner to within a main var statement.

Renamed imgResSel to imgResolutionMenu.

12/03/2015
Full menu scaling capabilities added for all views.

Added a flag, this._isFirstRender, which ensures that the original image dimensions will only be calculated and cached once when user enters the diff modes. 

Renamed variables to suit JS naming conventions (changed underscore case to camel case).

Removed redundant parseFloats within functions which were already taking in floats.

Testing Done

Ensured that images are resizing correctly on the browser.

Files

Issues

Description	From	Last Updated
Something is wrong with that tab header. I've attached a screenshot that shows the difference between it and the regular …	brennie	Nov. 14, 2015, 12:21 a.m.
Can this text be black instead of blue?	brennie	Nov. 15, 2015, 2:16 p.m.
How about #img-resolution-menu	brennie	Nov. 6, 2015, 12:19 p.m.
These should be alphabetized.	brennie	Nov. 6, 2015, 12:54 p.m.
I have a few concerns about this. This is rather unreadable. It would be more readable if we broke it …	brennie	Nov. 13, 2015, 11:49 a.m.
var statements go at the top of the function	brennie	Nov. 18, 2015, 11:58 a.m.
Likewise here. We also avoid doing self = this style bindings and prefer using _.bind instead. However, this doesn't appear …	brennie	Nov. 15, 2015, 2:19 p.m.
/*	brennie	Nov. 14, 2015, 8:35 a.m.
Needs docstring.	brennie	Nov. 15, 2015, 2:19 p.m.
Remove blank line.	brennie	Nov. 15, 2015, 2:19 p.m.
Single var statement at the top of the function. We should be caching the original width and height on the …	brennie	Nov. 15, 2015, 2:19 p.m.
Currently, each time you press the button, the image is going to be twice as large as the previous press, …	brennie	Nov. 15, 2015, 2:19 p.m.
Should be formatted as: /* * Single line summary. * * Multi-line description. */	brennie	Nov. 17, 2015, 12:39 p.m.
Remove this line.	brennie	Nov. 16, 2015, 12:35 p.m.
See above.	brennie	Nov. 17, 2015, 12:39 p.m.
Remove this line.	brennie	Nov. 16, 2015, 12:34 p.m.
Isn't this in your other patch? If so, please remove this.	brennie	Nov. 17, 2015, 1:03 p.m.
Remove this line.	brennie	Nov. 16, 2015, 12:33 p.m.
This can all be done in initialize.	brennie	Nov. 17, 2015, 12:39 p.m.
You should cache the image result, e.g. var img = $('.image-diff-attachment'), width = img.width(), height = img.height(); Also, how does …	brennie	Nov. 17, 2015, 12:41 p.m.
JavaScript doesn't support multiple returns.	brennie	Nov. 18, 2015, 12:11 p.m.
Remove this line.	brennie	Nov. 16, 2015, 12:33 p.m.
Can we change this to changeScale and have it only take the scale? We should be able to get width …	brennie	Nov. 28, 2015, 8:19 a.m.
If we cache the image on the object as _$img in initialize, we can do: this._$img.css({ width: this._width * scale, …	brennie	Nov. 18, 2015, 12:30 p.m.
Remove this.	brennie	Nov. 16, 2015, 12:32 p.m.
Lets call this data-image-scale.	brennie	Nov. 17, 2015, 12:42 p.m.
ES5 doesn't support destructuring.	brennie	Nov. 18, 2015, 12:12 p.m.
Needs a doc comment. Can we call it _onChangeScaleClicked ?	brennie	Nov. 17, 2015, 12:44 p.m.
Please replace this with var scale = parseFloat($(e.target).data('iamge-scale')); This ensures we get the correct attribute (image-scale) vs the first attribute …	brennie	Nov. 17, 2015, 12:53 p.m.
This needs a better name.	brennie	Nov. 28, 2015, 8:27 a.m.
This should be one var statement, e.g. var hasDiff = this.model.get('diffAgainstFileAttachmentID !== null'), captionItems = [], $header, $headerInner, $revisionLabel, $revisionSelector;	brennie	Nov. 28, 2015, 8:27 a.m.
Can we not use self = this here? CAn you replace all functions that use self to use this and …	brennie	Nov. 21, 2015, 10:07 a.m.
How about: $headerInner = $('<div class="review-ui-header-inner" />') .html(this.imgResSel) .appendTo($header);	brennie	Nov. 28, 2015, 8:34 a.m.
Undo this.	brennie	Nov. 28, 2015, 8:31 a.m.
Blank line between these.	brennie	Nov. 28, 2015, 8:35 a.m.
We don't want to append to $header until its ready to display, so we should do: $headerInner = $('...'); if …	brennie	Dec. 3, 2015, 4:12 p.m.
Blank line between these.	brennie	Nov. 28, 2015, 8:35 a.m.
Use /* ... */ for multi-line comments.	brennie	Nov. 28, 2015, 8:36 a.m.
Use /* ... */ for multi-line comments.	brennie	Nov. 28, 2015, 8:36 a.m.
This should probably return something	david	Nov. 30, 2015, 4:45 p.m.
Missing a space between * and Changes.	david	Dec. 1, 2015, 11:58 a.m.
We combine all var assignments into a single statement. That said, it's probably better to do this: var $image = …	david	Dec. 1, 2015, 12:27 p.m.
We should require that people calling this pass in a float, rather than calling parseFloat here.	david	Dec. 1, 2015, 12:06 p.m.
Undo this, please.	david	Dec. 1, 2015, 12:01 p.m.
These should all be a single statement. Also, variable names in javascript code should be origWidth rather than orig_width.	david	Dec. 3, 2015, 4:15 p.m.
Leftover debug output?	david	Dec. 1, 2015, 12:01 p.m.
Same here re: parseFloat usage.	david	Dec. 1, 2015, 12:07 p.m.
Undo this, please.	david	Dec. 1, 2015, 12:02 p.m.
Multi-line comments should be formatted as: /* * The default ... * original ... * carries ... */	david	Dec. 1, 2015, 12:02 p.m.
I'd really prefer to not hard-code information about the views here. Since the dimensions of the source images should be …	david	Dec. 3, 2015, 4:11 p.m.
Use === instead of ==.	david	Dec. 2, 2015, 11:44 a.m.
Oh, you're already calling parseFloat here, so you can just remove it from the changeScale implementations.	david	Dec. 1, 2015, 12:07 p.m.
Same here re: multi-line comments. That said, this comment doesn't add a ton of info (maybe where you define this._dimensions …	david	Dec. 2, 2015, 12:18 p.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less

Change Summary:

Changed title of review request.

Summary:

-	Added drop down menu for image scaling.
+	WIP: High DPI Image Review

reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)
The issue has been resolved. Show all issues
```
How about #img-resolution-menu
```

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

The issue has been resolved. Show all issues

These should be alphabetized.

chronicleyu Nov. 6, 2015, 12:27 p.m.

Is this what you mean?
list-style-type: lower-alpha;

david Nov. 6, 2015, 12:47 p.m.

He meant alphabetize the rules (float, then list-style, then margin, then padding, then width, then z-index).

CH

chronicleyu Nov. 6, 2015, 12:54 p.m.
```
Gotcha. Fixed.
```

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

The issue has been resolved. Show all issues

I have a few concerns about this.


This is rather unreadable. It would be more readable if we broke it out into a string on the class and then we can use that to render the menu. Using a template also allows for easy extensibility later on.


The <a> items lack an easy to use selector to hook into. It is probably best to use the same class on each <a> and store the desired resolution in a data- attribute.


The ID #imgrevnav doesn't really explain what this does. This should be something like #img-resolution-selector.


You don't need to use a nested <ul> structure. The menu wrapper can just be a <div>. This will simplify your CSS quite a bit.


$imageScale is also not very descriptive. $imageScaleMenu would be better.


There is no var statement for $imageScale.


An example of all the suggestions above follows:

{
    html: [
        '<div id="#img-resolution-selector">',
        ' <a href="#">',
        '  <span class="fa fa-search-plus"></span>',
        '  <span id="#img-resolution-current-zoom>100%</span>',
        ' </a>',
        ' <ul>',
        '  <li><a href="#" class="change-img-resolution" data-resolution="0.33">33%</span></li>',
        '  <li><a href="#" class="change-img-resolution" data-resolution="0.50">50%</span></li>',
        '  <li><a href="#" class="change-img-resolution" data-reolsution="1.0">100$</span></li>',
        ' </ul>',
        '</div>'
    ]).join('')
}

// ...

$imageScaleMenu = $(this.html).appendTo($header);

brennie Nov. 5, 2015, 8:18 p.m.
```
That should read 100% not 100$ above.
```

Change Summary:

Resolved menu positioning issue. Added CSS hovering feedback to the menu. Changed name of CSS reference to the menu.

Testing Done:

		11/04/2015

		WIP of the drop down menu for selecting image scale.
~		*Problem - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*
	~	*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*


	+
	+	11/07/2015
	+
	+	Resolved menu positioning problem (Problem # 1)
	+	Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)
	+
	+	Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.

Diff:

Revision 2 (+102 -1)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/imageReviewableView.js

Removed Files:

Added Files:

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less

Change Summary:

Reformatted html code to improve readability. Added event binding to menu. Tested event binding by hard coding image re-sizing.

Summary:

-	WIP: High DPI Image Review
+	Code Dump: High DPI Image Review

Description:

~		WIP: High DPI Image Review
	~	Code Dump: High DPI Image Review

Testing Done:

		11/04/2015

		WIP of the drop down menu for selecting image scale.
		*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*



		11/07/2015

		Resolved menu positioning problem (Problem # 1)
		Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)

~		Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.
	~	(RESOLVED - 11/07/2015) Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.
	+
	+
	+
	+	11/13/2015
	+
	+	Re-formatted menu html code to be more readable.
	+	Added event binding to the menu options.
	+	Hard coded the event's function to increase image size by 200%.
	+
	+

Diff:

Revision 3 (+142 -4)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/imageReviewableView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less

Change Summary:

Removed some previously written code which is no longer necessary.

Diff:

Revision 4 (+132 -4)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/imageReviewableView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less

Fix it!

highdpi_img_menu.png (Revision 1)

An issue was opened. Show all issues

Something is wrong with that tab header. I've attached a screenshot that shows the difference between it and the regular header:

screenshot

chronicleyu Nov. 15, 2015, 2:17 p.m.

Not sure what's happening here. Will look into it further.

highdpi_img_menu.png (Revision 1)
The issue has been resolved. Show all issues
```
Can this text be black instead of blue?
```

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

The issue has been resolved. Show all issues

var statements go at the top of the function

chronicleyu Nov. 15, 2015, 2:16 p.m.

Header disappears when this particular var statement is moved to the top.

brennie Nov. 16, 2015, 9:30 a.m.

You can declare the variable an instantiate it here. It must be at the top.

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

The issue has been resolved. Show all issues

Likewise here. We also avoid doing self = this style bindings and prefer using _.bind instead. However, this doesn't appear to be doing anything.

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
/*
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Needs docstring.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Remove blank line.
```

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

The issue has been resolved. Show all issues

Single var statement at the top of the function.
We should be caching the original width and height on the object (in initialize).

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

The issue has been resolved. Show all issues

Currently, each time you press the button, the image is going to be twice as large as the previous press, and each time you press it should get bigger.

Change Summary:

Moved description from Testing Done to Description. Added a note to most recent update to description.

Description:

~		Code Dump: High DPI Image Review
	~	Code Dump: High DPI Image Review
	+
	+	11/04/2015
	+
	+	WIP of the drop down menu for selecting image scale.
	+	*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*
	+
	+
	+
	+	11/07/2015
	+
	+	Resolved menu positioning problem (Problem # 1)
	+	Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)
	+
	+	(RESOLVED - 11/07/2015) Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.
	+
	+
	+
	+	11/13/2015
	+
	+	Re-formatted menu html code to be more readable.
	+	Added event binding to the menu options.
	+	Hard coded the event's function to increase image size by 200%.
	+
	+	NOTE: The week's goal was to try and understand the RB's implementation of Backbone.js and jQuery, and to try and get any kind of change in size of the image. So for now, clicking on the menu options will keep increasing the image size by 200%. Will begin working on the actual implementation of the buttons starting this week.
	+
	+

Testing Done:

-		11/04/2015
-
-		WIP of the drop down menu for selecting image scale.
-		*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*
-
-
-
-		11/07/2015
-
-		Resolved menu positioning problem (Problem # 1)
-		Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)
-
-		(RESOLVED - 11/07/2015) Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.
-
-
-
-		11/13/2015
-
-		Re-formatted menu html code to be more readable.
-		Added event binding to the menu options.
-		Hard coded the event's function to increase image size by 200%.
-
-

Change Summary:

Menu scaling now fully working for ImageAttachmentView. Also includes minor code fixes and a CSS change.

Description:

		Code Dump: High DPI Image Review

		11/04/2015

		WIP of the drop down menu for selecting image scale.
		*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*



		11/07/2015

		Resolved menu positioning problem (Problem # 1)
		Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)

		(RESOLVED - 11/07/2015) Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.



		11/13/2015

		Re-formatted menu html code to be more readable.
		Added event binding to the menu options.
		Hard coded the event's function to increase image size by 200%.

		NOTE: The week's goal was to try and understand the RB's implementation of Backbone.js and jQuery, and to try and get any kind of change in size of the image. So for now, clicking on the menu options will keep increasing the image size by 200%. Will begin working on the actual implementation of the buttons starting this week.


	+
	+	11/14/2015
	+
	+	Full menu scaling capabilities added for ImageAttachmentView.
	+	Minor rearrangement of code.
	+	Change menu text color from blue to black.

Testing Done:

Test server and browser.

Diff:

Revision 5 (+184 -3)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/imageReviewableView.js

Removed Files:

Added Files:

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less

Change Summary:

Corrected the date of the latest addition to Description.

Description:

		Code Dump: High DPI Image Review

		11/04/2015

		WIP of the drop down menu for selecting image scale.
		*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*



		11/07/2015

		Resolved menu positioning problem (Problem # 1)
		Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)

		(RESOLVED - 11/07/2015) Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.



		11/13/2015

		Re-formatted menu html code to be more readable.
		Added event binding to the menu options.
		Hard coded the event's function to increase image size by 200%.

		NOTE: The week's goal was to try and understand the RB's implementation of Backbone.js and jQuery, and to try and get any kind of change in size of the image. So for now, clicking on the menu options will keep increasing the image size by 200%. Will begin working on the actual implementation of the buttons starting this week.



~		11/14/2015
	~	11/15/2015

		Full menu scaling capabilities added for ImageAttachmentView.
		Minor rearrangement of code.
		Change menu text color from blue to black.

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

The issue has been resolved. Show all issues

Should be formatted as:

/*
 * Single line summary.
 *
 * Multi-line description.
 */

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Remove this line.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
See above.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Remove this line.
```

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

The issue has been resolved. Show all issues

Isn't this in your other patch? If so, please remove this.

chronicleyu Nov. 16, 2015, 12:15 p.m.

This would mean every jQuery reference to image-diff-attachment in my code for this project would become image-diff-null for things to work properly, and then we'd have to write a patch to fix this after the other patch goes live (presumably before this one).

I'm new to web dev so I'm having trouble wrapping my head around the whole parallel timeline.

chronicleyu Nov. 16, 2015, 12:22 p.m.

Please ignore previous reply. I will rebase r/7754 off of master.

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Remove this line.
```

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

The issue has been resolved. Show all issues

This can all be done in initialize.

chronicleyu Nov. 16, 2015, 12:07 p.m.

I initially thought about that, but some modes (e.g. ImageDifferenceDiffView) involve multiple images, so getting the starting dimensions can get messy if it's all done in initialize. This is why it may be better to have this as a function in the base class which subclasses can override and then have initialize call the function. 
Right now, the render function calls getStartingSize(), but I can move that into initialize. Thoughts?

brennie Nov. 16, 2015, 12:20 p.m.
```
render is a fine place to put it.
```

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

The issue has been resolved. Show all issues

You should cache the image result, e.g.

var img = $('.image-diff-attachment'),
    width = img.width(),
    height = img.height();


Also, how does this.$el relate to $('.image-diff-attachment')?
We should probably cache the img on the object with this.$('img')

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
JavaScript doesn't support multiple returns.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Remove this line.
```

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

The issue has been resolved. Show all issues

Can we change this to changeScale and have it only take the scale? We should be able to get width and height from this.

chronicleyu Nov. 18, 2015, 12:54 p.m.

Would the function have to take this: changeSize: function(size, this) {...
Or can I call this._width and this._height within the function without doing that?

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

The issue has been resolved. Show all issues

If we cache the image on the object as _$img in initialize, we can do:

this._$img.css({
   width: this._width * scale,
   height: this._height * scale
});

which is much more succinct and extensible.

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Remove this.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Lets call this data-image-scale.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
ES5 doesn't support destructuring.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs a doc comment.
Can we call it _onChangeScaleClicked ?
```

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

The issue has been resolved. Show all issues

Please replace this with

var scale = parseFloat($(e.target).data('iamge-scale'));


This ensures we get the correct attribute (image-scale) vs the first attribute on the element, as well as parsing it into a float so we can use it in computations.

Change Summary:

Added WIP on implementing image scaling to OnionDiff mode. Modified changeScale overriding to take in an array of dimensions vs just width and height.

Description:

		Code Dump: High DPI Image Review

		11/04/2015

		WIP of the drop down menu for selecting image scale.
		*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*



		11/07/2015

		Resolved menu positioning problem (Problem # 1)
		Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)

		(RESOLVED - 11/07/2015) Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.



		11/13/2015

		Re-formatted menu html code to be more readable.
		Added event binding to the menu options.
		Hard coded the event's function to increase image size by 200%.

		NOTE: The week's goal was to try and understand the RB's implementation of Backbone.js and jQuery, and to try and get any kind of change in size of the image. So for now, clicking on the menu options will keep increasing the image size by 200%. Will begin working on the actual implementation of the buttons starting this week.



		11/15/2015

		Full menu scaling capabilities added for ImageAttachmentView.
		Minor rearrangement of code.
		Change menu text color from blue to black.
	+
	+
	+
	+	11/21/2015
	+
	+	WIP on getting scaling to work on OnionDiff mode. It is proving to be more challenging than imageAttachmentView mode because it looks like I'll have to change the CSS of multiple classes rather than just one.
	+
	+	Problem # 3 - When there are diffs, I can't get the menu to appear in line with the image file names. I also tried putting the menu in the bar with the mode selections but that is giving me positional problems too.
	+
	+	Moved `var` statement for $headerInner to top of function.
	+	Standardized the formatting of function documentation.
	+	Removal of unnecessary lines and statements.

Diff:

Revision 6 (+234 -7)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/imageReviewableView.js

Added Files:

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less

Fix it!

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
This needs a better name.
```

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

The issue has been resolved. Show all issues

This should be one var statement, e.g.

var hasDiff = this.model.get('diffAgainstFileAttachmentID !== null'),
    captionItems = [],
    $header,
    $headerInner,
    $revisionLabel,
    $revisionSelector;

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

An issue was opened. Show all issues

Can we not use self = this here?

CAn you replace all functions that use self to use this and call _.bind(fn, this) with them?

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

An issue was opened. Show all issues

How about:

$headerInner = $('<div class="review-ui-header-inner" />')
    .html(this.imgResSel)
    .appendTo($header);

chronicleyu Nov. 28, 2015, 10:52 a.m.

The scaling menu disappeared when I tried this.

david Nov. 30, 2015, 4:36 p.m.

Well, it ought to work, so something is broken.

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Undo this.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank line between these.
```

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

The issue has been resolved. Show all issues

We don't want to append to $header until its ready to display, so we should do:

$headerInner = $('...');

if () {
   // ...
}

$headerInner
    .html(this.imgResSel)
    .appendTo($header);

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Use /* ... */ for multi-line comments.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Use /* ... */ for multi-line comments.
```

Change Summary:

Added scaling functionality to two diff modes: Two Up and Onion
Added fixes for most of the reviews on the previous RR.

Description:

		Code Dump: High DPI Image Review

		11/04/2015

		WIP of the drop down menu for selecting image scale.
		*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*



		11/07/2015

		Resolved menu positioning problem (Problem # 1)
		Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)

		(RESOLVED - 11/07/2015) Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.



		11/13/2015

		Re-formatted menu html code to be more readable.
		Added event binding to the menu options.
		Hard coded the event's function to increase image size by 200%.

		NOTE: The week's goal was to try and understand the RB's implementation of Backbone.js and jQuery, and to try and get any kind of change in size of the image. So for now, clicking on the menu options will keep increasing the image size by 200%. Will begin working on the actual implementation of the buttons starting this week.



		11/15/2015

~		Full menu scaling capabilities added for ImageAttachmentView.
	~	Full menu scaling capabilities added for `ImageAttachmentView`.
		Minor rearrangement of code.
		Change menu text color from blue to black.



		11/21/2015

		WIP on getting scaling to work on OnionDiff mode. It is proving to be more challenging than imageAttachmentView mode because it looks like I'll have to change the CSS of multiple classes rather than just one.

		Problem # 3 - When there are diffs, I can't get the menu to appear in line with the image file names. I also tried putting the menu in the bar with the mode selections but that is giving me positional problems too.

		Moved `var` statement for $headerInner to top of function.
		Standardized the formatting of function documentation.
		Removal of unnecessary lines and statements.
	+
	+
	+
	+	11/28/2015
	+
	+	Full menu scaling capabilities added for `ImageTwoUpDiffView` and `ImageOnionDiffView`.
	+	Moved `var` statement for `$headerInner` to within a main `var` statement.
	+	Renamed `imgResSel` to `imgResolutionMenu`.

Testing Done:

~		Test server and browser.
	~	Ensured that images are resizing correctly on the browser.

Diff:

Revision 7 (+260 -5)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/imageReviewableView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less

Fix it!

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

An issue was opened. Show all issues

This should probably return something

chronicleyu Dec. 3, 2015, 4:30 p.m.

This base class function is used via overrides to return an array of length 2 for ImageAttachmentView,

and an array of length 4 for the diff modes, so I'm unsure about what it should return here.
I saw that the onImagesLoaded function just before it was also left empty (since other classes override

it as well), so I followed that.

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Missing a space between * and Changes.
```

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

The issue has been resolved. Show all issues

We combine all var assignments into a single statement. That said, it's probably better to do this:

var $image = this.$('.image-diff-attachment');
return [$image.width(), $image.height()];

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
We should require that people calling this pass in a float, rather than calling parseFloat here.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Undo this, please.
```

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

The issue has been resolved. Show all issues

These should all be a single statement. Also, variable names in javascript code should be origWidth rather than orig_width.

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Leftover debug output?
```

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

The issue has been resolved. Show all issues

Same here re: parseFloat usage.

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Undo this, please.
```

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

The issue has been resolved. Show all issues

Multi-line comments should be formatted as:

/*
 * The default ...
 * original ...
 * carries ...
 */

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

The issue has been resolved. Show all issues

I'd really prefer to not hard-code information about the views here. Since the dimensions of the source images should be constant, can't we just get it once and not have to re-fetch it here?

chronicleyu Dec. 2, 2015, 11:57 a.m.

Agreed. This is also a problem because when you return to 'two-up' mode AFTER changing the scale in another mode, the new dimensions become the new 100%, and we don't want that happening. I'm thinking of ways of getting and caching the dimensions once.

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Use === instead of ==.
```

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

The issue has been resolved. Show all issues

Oh, you're already calling parseFloat here, so you can just remove it from the changeScale implementations.

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

The issue has been resolved. Show all issues

Same here re: multi-line comments. That said, this comment doesn't add a ton of info (maybe where you define this._dimensions explain what the potential type(s) are?).

Change Summary:

Added full scaling functionality for all modes. Added fixes for the majority of reviews on the previous RR.

Description:

		Code Dump: High DPI Image Review

		11/04/2015

		WIP of the drop down menu for selecting image scale.
		*(RESOLVED - 11/07/2015) Problem # 1 - Can't get menu to appear in the center and just to the right of the image filename (see mockup image)*



		11/07/2015

		Resolved menu positioning problem (Problem # 1)
		Fixed a couple of issues raised from reviews about the CSS code. (1 unresolved review)

		(RESOLVED - 11/07/2015) Problem # 2 - I put a `console.log(this.$el);` into the ImageAttachmentView render function, which is basically the corresponding view to the screenshot attached, yet nothing appears on the console even when the view is being generated.



		11/13/2015

		Re-formatted menu html code to be more readable.
		Added event binding to the menu options.
		Hard coded the event's function to increase image size by 200%.

		NOTE: The week's goal was to try and understand the RB's implementation of Backbone.js and jQuery, and to try and get any kind of change in size of the image. So for now, clicking on the menu options will keep increasing the image size by 200%. Will begin working on the actual implementation of the buttons starting this week.



		11/15/2015

		Full menu scaling capabilities added for `ImageAttachmentView`.
		Minor rearrangement of code.
		Change menu text color from blue to black.



		11/21/2015

		WIP on getting scaling to work on OnionDiff mode. It is proving to be more challenging than imageAttachmentView mode because it looks like I'll have to change the CSS of multiple classes rather than just one.

		Problem # 3 - When there are diffs, I can't get the menu to appear in line with the image file names. I also tried putting the menu in the bar with the mode selections but that is giving me positional problems too.

		Moved `var` statement for $headerInner to top of function.
		Standardized the formatting of function documentation.
		Removal of unnecessary lines and statements.



		11/28/2015

		Full menu scaling capabilities added for `ImageTwoUpDiffView` and `ImageOnionDiffView`.
		Moved `var` statement for `$headerInner` to within a main `var` statement.
		Renamed `imgResSel` to `imgResolutionMenu`.
	+
	+
	+
	+	12/03/2015
	+
	+	Full menu scaling capabilities added for all views.
	+	Added a flag, `this._isFirstRender`, which ensures that the original image dimensions will only be calculated and cached once when user enters the diff modes.
	+	Renamed variables to suit JS naming conventions (changed underscore case to camel case).
	+	Removed redundant `parseFloat`s within functions which were already taking in floats.

Diff:

Revision 8 (+306 -7)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/imageReviewableView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/static/rb/css/pages/reviews.less

Status: Discarded

Change Summary:

Discarded in favor of /r/8216/

Code Dump: High DPI Image Review

Information

Reviewers