Code Dump: High DPI Image Review
Review Request #7754 — Created Nov. 4, 2015 and discarded
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
andImageOnionDiffView
.
Movedvar
statement for$headerInner
to within a mainvar
statement.
RenamedimgResSel
toimgResolutionMenu
.
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 redundantparseFloat
s within functions which were already taking in floats.
Ensured that images are resizing correctly on the browser.
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 | |
Can this text be black instead of blue? |
brennie | |
How about #img-resolution-menu |
brennie | |
These should be alphabetized. |
brennie | |
I have a few concerns about this. This is rather unreadable. It would be more readable if we broke it … |
brennie | |
var statements go at the top of the function |
brennie | |
Likewise here. We also avoid doing self = this style bindings and prefer using _.bind instead. However, this doesn't appear … |
brennie | |
/* |
brennie | |
Needs docstring. |
brennie | |
Remove blank line. |
brennie | |
Single var statement at the top of the function. We should be caching the original width and height on the … |
brennie | |
Currently, each time you press the button, the image is going to be twice as large as the previous press, … |
brennie | |
Should be formatted as: /* * Single line summary. * * Multi-line description. */ |
brennie | |
Remove this line. |
brennie | |
See above. |
brennie | |
Remove this line. |
brennie | |
Isn't this in your other patch? If so, please remove this. |
brennie | |
Remove this line. |
brennie | |
This can all be done in initialize. |
brennie | |
You should cache the image result, e.g. var img = $('.image-diff-attachment'), width = img.width(), height = img.height(); Also, how does … |
brennie | |
JavaScript doesn't support multiple returns. |
brennie | |
Remove this line. |
brennie | |
Can we change this to changeScale and have it only take the scale? We should be able to get width … |
brennie | |
If we cache the image on the object as _$img in initialize, we can do: this._$img.css({ width: this._width * scale, … |
brennie | |
Remove this. |
brennie | |
Lets call this data-image-scale. |
brennie | |
ES5 doesn't support destructuring. |
brennie | |
Needs a doc comment. Can we call it _onChangeScaleClicked ? |
brennie | |
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 | |
This needs a better name. |
brennie | |
This should be one var statement, e.g. var hasDiff = this.model.get('diffAgainstFileAttachmentID !== null'), captionItems = [], $header, $headerInner, $revisionLabel, $revisionSelector; |
brennie | |
Can we not use self = this here? CAn you replace all functions that use self to use this and … |
brennie | |
How about: $headerInner = $('<div class="review-ui-header-inner" />') .html(this.imgResSel) .appendTo($header); |
brennie | |
Undo this. |
brennie | |
Blank line between these. |
brennie | |
We don't want to append to $header until its ready to display, so we should do: $headerInner = $('...'); if … |
brennie | |
Blank line between these. |
brennie | |
Use /* ... */ for multi-line comments. |
brennie | |
Use /* ... */ for multi-line comments. |
brennie | |
This should probably return something |
david | |
Missing a space between * and Changes. |
david | |
We combine all var assignments into a single statement. That said, it's probably better to do this: var $image = … |
david | |
We should require that people calling this pass in a float, rather than calling parseFloat here. |
david | |
Undo this, please. |
david | |
These should all be a single statement. Also, variable names in javascript code should be origWidth rather than orig_width. |
david | |
Leftover debug output? |
david | |
Same here re: parseFloat usage. |
david | |
Undo this, please. |
david | |
Multi-line comments should be formatted as: /* * The default ... * original ... * carries ... */ |
david | |
I'd really prefer to not hard-code information about the views here. Since the dimensions of the source images should be … |
david | |
Use === instead of ==. |
david | |
Oh, you're already calling parseFloat here, so you can just remove it from the changeScale implementations. |
david | |
Same here re: multi-line comments. That said, this comment doesn't add a ton of info (maybe where you define this._dimensions … |
david |
-
-
-
-
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 sameclass
on each<a>
and store the desired resolution in adata-
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);
-
- 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)
- 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 ReviewCode 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%. + +
-
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
-
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
-
-
Something is wrong with that tab header. I've attached a screenshot that shows the difference between it and the regular header:
-
-
-
Likewise here. We also avoid doing
self = this
style bindings and prefer using_.bind
instead. However, this doesn't appear to be doing anything. -
-
-
-
Single
var
statement at the top of the function.We should be caching the original
width
andheight
on the object (ininitialize
). -
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
toDescription
. 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)
- 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.
-
-
-
-
-
-
-
-
-
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 withthis.$('img')
-
-
-
Can we change this to
changeScale
and have it only take the scale? We should be able to get width and height fromthis
. -
If we cache the image on the object as
_$img
ininitialize
, we can do:this._$img.css({ width: this._width * scale, height: this._height * scale });
which is much more succinct and extensible.
-
-
-
-
-
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)
- 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
-
-
-
This should be one
var
statement, e.g.var hasDiff = this.model.get('diffAgainstFileAttachmentID !== null'), captionItems = [], $header, $headerInner, $revisionLabel, $revisionSelector;
-
Can we not use
self = this
here?CAn you replace all functions that use
self
to usethis
and call_.bind(fn, this)
with them? -
How about:
$headerInner = $('<div class="review-ui-header-inner" />') .html(this.imgResSel) .appendTo($header);
-
-
-
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);
-
-
-
- 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
andImageOnionDiffView
.+ Moved var
statement for$headerInner
to within a mainvar
statement.+ Renamed imgResSel
toimgResolutionMenu
. - Testing Done:
-
~ Test server and browser.
~ Ensured that images are resizing correctly on the browser.
-
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
-
-
-
-
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()];
-
-
-
These should all be a single statement. Also, variable names in javascript code should be
origWidth
rather thanorig_width
. -
-
-
-
-
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?
-
-
Oh, you're already calling
parseFloat
here, so you can just remove it from thechangeScale
implementations. -
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
andImageOnionDiffView
.Moved var
statement for$headerInner
to within a mainvar
statement.Renamed imgResSel
toimgResolutionMenu
.+ + + + 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.