Summary:

Review UI for SVG Vector Graphics files

Review Request #11846 — Created Oct. 8, 2021 and updated Dec. 6, 2021, 3:40 a.m.

Information
Owner:	akim.ruslanov
Repository:	Review Board
Branch:	release-4.0.x
Bugs:
Depends On:
Reviewers
Groups:	students
People:

Description

Currently ReviewBoard has a limited way of rendering SVG files and does
not allow for proper scaling and zooming. We are now attempting to utilize
SVG potential to full extent and allow users to inifinitely scale/zoom SVG images.

The new solution utilizes viewBox attribute of SVGs and attempts to manipulate it
in order to create zoom-in ability and ability to span SVG. Moreover, the solution
also dynamically rescales and adjusts comment boxes based on the viewBox.

Currently, the new features only apply to a single image, i.e. no diffs view.
The diff view with SVGs will behave the same way as with the bitmap images.
However, there is no ability to zoom in/out or span the SVG.

Testing Done

The UI was tested using Google Chrome and Safari.

Unit tests were but no additional tests were added.

Commits

Summary	Author
wip initial	aruslanov@fispan.com
mousewheel works in parent view	aruslanov@fispan.com
clean up	aruslanov@fispan.com
fixed event handling	aruslanov@fispan.com
onload question	aruslanov@fispan.com
fixed jshint problems	aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
fixed jshint issues	aruslanov@fispan.com
figuring out fetching within render	aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
fixing some jshint issues	aruslanov@fispan.com
added documentation; cleaned up code	aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
fixed more whitespaces	aruslanov@fispan.com
small bug with new method	aruslanov@fispan.com
mend	aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
missed out on some small whitespaces and semicolons	aruslanov@fispan.com
comments are scaled with SVGs	aruslanov@fispan.com
fixing whitespaces	aruslanov@fispan.com
jshint issues fixed	aruslanov@fispan.com
i hate whitespaces	aruslanov@fispan.com

Screenshots

Files

Issues

Description	From	Last Updated
The new toolbar seems to be SVG-specific. It would be ideal if the ability to zoom, pan, etc. could be …	chipx86	Dec. 6, 2021, 3:40 a.m.
Is this an expected behaviour, or should the comment also change its position based on the zoom in/out of SVG?	akim.ruslanov	Nov. 29, 2021, 12:48 a.m.
In UI, discoverability is important. A person should ideally be able to see what options are available before having to …	chipx86	Dec. 6, 2021, 3:40 a.m.
Col: 44 Missing semicolon.	reviewbot	Oct. 14, 2021, 12:31 a.m.
Col: 30 Missing semicolon.	reviewbot	Oct. 14, 2021, 12:31 a.m.
Col: 44 ['width'] is better written in dot notation.	reviewbot	Oct. 14, 2021, 12:32 a.m.
Col: 44 ['height'] is better written in dot notation.	reviewbot	Oct. 14, 2021, 12:32 a.m.
Col: 43 ['width'] is better written in dot notation.	reviewbot	Oct. 14, 2021, 12:32 a.m.
Col: 44 ['height'] is better written in dot notation.	reviewbot	Oct. 14, 2021, 12:32 a.m.
Col: 15 '$image' is defined but never used.	reviewbot	Oct. 14, 2021, 12:33 a.m.
Col: 63 Expected '===' and instead saw '=='.	reviewbot	Oct. 14, 2021, 12:33 a.m.
This is listening for events on an element with the image-diff-attachment which is a child of the current view, but …	david	Oct. 28, 2021, 7:08 p.m.
This event is getting ignored completely and I am not super sure why. The only reasoning I could find was …	akim.ruslanov	Oct. 28, 2021, 7:10 p.m.
This event works. I have tried this because I wanted to see if mousewheel event works at all. So my …	akim.ruslanov	Oct. 28, 2021, 7:10 p.m.
Col: 57 Missing semicolon.	reviewbot	Nov. 2, 2021, 12:06 a.m.
Col: 13 'imageSrc' is defined but never used.	reviewbot	Nov. 2, 2021, 12:06 a.m.
Col: 39 Missing semicolon.	reviewbot	Nov. 2, 2021, 12:07 a.m.
Col: 43 Missing semicolon.	reviewbot	Nov. 2, 2021, 12:07 a.m.
Col: 23 Missing semicolon.	reviewbot	Nov. 2, 2021, 12:10 a.m.
Col: 14 Missing semicolon.	reviewbot	Nov. 2, 2021, 12:10 a.m.
Col: 11 Missing semicolon.	reviewbot	Nov. 2, 2021, 12:10 a.m.
This function's sole purpose is to change viewBox value for the SVG based on the values passed from the parent …	akim.ruslanov	Dec. 1, 2021, 7 p.m.
Col: 19 '$image' is defined but never used.	reviewbot	Nov. 18, 2021, 6:44 p.m.
Col: 14 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:44 p.m.
Col: 23 '$image' is defined but never used.	reviewbot	Nov. 18, 2021, 6:44 p.m.
Col: 18 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:44 p.m.
Col: 23 '$image' is defined but never used.	reviewbot	Nov. 18, 2021, 6:44 p.m.
Col: 18 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:44 p.m.
Col: 38 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:44 p.m.
Col: 19 '$image' is defined but never used.	reviewbot	Nov. 18, 2021, 6:46 p.m.
Col: 14 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:47 p.m.
Col: 23 '$image' is defined but never used.	reviewbot	Nov. 18, 2021, 6:47 p.m.
Col: 18 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:48 p.m.
Col: 23 '$image' is defined but never used.	reviewbot	Nov. 18, 2021, 6:48 p.m.
Col: 18 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:55 p.m.
Col: 59 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:55 p.m.
Col: 61 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:58 p.m.
Col: 54 Missing semicolon.	reviewbot	Nov. 18, 2021, 6:59 p.m.
Col: 38 Missing semicolon.	reviewbot	Nov. 18, 2021, 7 p.m.
As far as I understand, the main purpose of loadImages and onImagesLoaded is to ensure data like initial-width and scaling …	akim.ruslanov	Dec. 1, 2021, 12:31 a.m.
In order to inline the SVG, I need to asynchronously fetch them and use the parser after. However, if I …	akim.ruslanov	Dec. 1, 2021, 7:03 p.m.
Col: 11 Missing semicolon.	reviewbot	Nov. 22, 2021, 6:07 p.m.
Col: 11 Missing semicolon.	reviewbot	Nov. 22, 2021, 6:07 p.m.
Col: 44 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:48 a.m.
Col: 38 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:48 a.m.
Col: 46 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:49 a.m.
Col: 11 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:53 a.m.
Col: 60 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:49 a.m.
Col: 62 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:50 a.m.
Col: 92 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:50 a.m.
Col: 130 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:50 a.m.
Col: 150 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:51 a.m.
Col: 15 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:51 a.m.
Col: 11 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:51 a.m.
Col: 72 Missing semicolon.	reviewbot	Nov. 29, 2021, 12:51 a.m.
Col: 46 Missing semicolon.	reviewbot	Nov. 29, 2021, 1:01 a.m.
Col: 72 Missing semicolon.	reviewbot	Nov. 29, 2021, 1:01 a.m.
Col: 46 Missing semicolon.	reviewbot	Dec. 1, 2021, 12:31 a.m.
Col: 40 Missing semicolon.	reviewbot	Dec. 1, 2021, 12:44 a.m.
Col: 72 Missing semicolon.	reviewbot	Dec. 1, 2021, 12:32 a.m.
Col: 19 'offset' is defined but never used.	reviewbot	Dec. 1, 2021, 12:32 a.m.
Col: 7 'SvgAttachmentView' is defined but never used.	reviewbot	Dec. 1, 2021, 12:59 a.m.
Col: 46 Missing semicolon.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 5 Expected '}' to match '{' from line 275 and instead saw 'getSelectionRegion'.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 23 Unorthodox function invocation.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 26 Expected ')' and instead saw '{'.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 27 Missing semicolon.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 1 Unexpected '}'.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 1 Expected ')' to match '(' from line 1 and instead saw '}'.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 1 Expected an assignment or function call and instead saw an expression.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 2 Missing semicolon.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 2 Expected an identifier and instead saw ')'.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 2 Expected an assignment or function call and instead saw an expression.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Col: 72 Missing semicolon.	reviewbot	Dec. 1, 2021, 12:49 a.m.
Please change #444 to #rb-ns-ui.colors[@grey-20]	david	Dec. 1, 2021, 6:06 p.m.
This should probably be set in initialize instead of here.	david	Dec. 1, 2021, 6:09 p.m.
Can we call this svgPromise, just so people definitely know that it's a promise and not data?	david	Dec. 1, 2021, 10:25 a.m.
This looks like it's not actually a jQuery object, so we shouldn't use the $ in the variable name.	david	Dec. 1, 2021, 6:11 p.m.
Let's split this up: this.svgSize = { width: this.viewBox.width, height: this.viewBox.height, };	david	Dec. 1, 2021, 6:12 p.m.
fetch already is a promise, so this whole thing can just be: return fetch(this.model.get('imageURL')) .then(res => res.text()) .then(data => { …	david	Dec. 1, 2021, 6:14 p.m.
Col: 46 Missing semicolon.	reviewbot	Dec. 1, 2021, 6:59 p.m.
What's going on here?	david	Dec. 1, 2021, 6:14 p.m.
Add a blank line between these.	david	Dec. 1, 2021, 6:15 p.m.
Too much indentation in here.	david	Dec. 1, 2021, 6:16 p.m.
Add a blank line between these.	david	Dec. 1, 2021, 6:16 p.m.
Can you explain this? Is this a TODO or FIXME? (if so, please use that text so it's searchable). This …	david	Dec. 1, 2021, 6:18 p.m.
This needs some blank lines to make it less dense. Comments also need to start with capital letters and end …	david	Dec. 1, 2021, 10:25 a.m.
Indent as such: this.viewBox = { x: ..., y: ..., };	david	Dec. 1, 2021, 6:20 p.m.
Add a trailing comma here.	david	Dec. 1, 2021, 6:21 p.m.
Add spaces around - operator.	david	Dec. 1, 2021, 6:21 p.m.
Line is too long. Please wrap to 80 columns throughout.	david	Dec. 1, 2021, 6:23 p.m.
svgFetched is already a promise, so we don't need to wrap with return new Promise(() => {})	david	Dec. 1, 2021, 6:24 p.m.
Typo?	david	Dec. 1, 2021, 6:25 p.m.
This shouldn't need async	david	Dec. 1, 2021, 10:25 a.m.
Did you test this with normal image attachments?	david	Dec. 1, 2021, 10:25 a.m.
We can use this.model.get('filename').endsWith('.svg')	david	Dec. 1, 2021, 6:37 p.m.
This needs a trailing comma.	david	Dec. 1, 2021, 6:29 p.m.
Parens and {} aren't necessary in here. Can just be .then(res => this._adjustPos())	david	Dec. 1, 2021, 6:29 p.m.
Col: 72 Missing semicolon.	reviewbot	Dec. 1, 2021, 6:59 p.m.
If you search the JS code for dedent you'll see a better way to write this literal.	david	Dec. 1, 2021, 10:25 a.m.
Add a blank line between these two.	david	Dec. 1, 2021, 6:37 p.m.
This can use addClass. Also, please use single quotes: this.$('#dragging-mode-icon').addClass('fa fa-arrows')	david	Dec. 1, 2021, 6:54 p.m.
Same here.	david	Dec. 1, 2021, 6:38 p.m.
Add a blank line between these two.	david	Dec. 1, 2021, 6:38 p.m.
And here.	david	Dec. 1, 2021, 6:38 p.m.
And here.	david	Dec. 1, 2021, 6:38 p.m.
There's an extra space character at the end of this line.	david	Dec. 1, 2021, 6:55 p.m.
This new blank line should be removed.	david	Dec. 1, 2021, 6:38 p.m.
This line is definitely too long. Please wrap to 80 columns throughout.	david	Dec. 1, 2021, 6:40 p.m.
This new blank line should be removed.	david	Dec. 1, 2021, 6:41 p.m.
This new blank line should be removed.	david	Dec. 1, 2021, 10:25 a.m.
Col: 66 Missing semicolon.	reviewbot	Dec. 1, 2021, 7 p.m.
Col: 32 Missing semicolon.	reviewbot	Dec. 1, 2021, 7:01 p.m.
Col: 19 'offset' is defined but never used.	reviewbot	Dec. 5, 2021, 6:48 p.m.
Col: 80 Missing semicolon.	reviewbot	Dec. 5, 2021, 6:48 p.m.
Col: 19 'offset' is defined but never used.	reviewbot	Dec. 5, 2021, 6:48 p.m.
Col: 19 'offset' is defined but never used.	reviewbot	Dec. 5, 2021, 6:48 p.m.
Col: 80 Missing semicolon.	reviewbot	Dec. 5, 2021, 6:49 p.m.
Col: 19 'offset' is defined but never used.	reviewbot	Dec. 5, 2021, 6:49 p.m.
Can you place this so that it's listed alphabetically with other classes?	chipx86	Dec. 6, 2021, 3:40 a.m.
There's a blank line here that you can get rid of.	chipx86	Dec. 6, 2021, 3:40 a.m.
Since we have svg as a local variable, use it here and everywhere else in this function instead of this.svg …	chipx86	Dec. 6, 2021, 3:40 a.m.
Comments should always end in a period.	chipx86	Dec. 6, 2021, 3:40 a.m.
When building an object, format it like: this.svgSize = { width: this.viewBox.width, height: this.viewBox.height, }; (Notice the trailing comma on …	chipx86	Dec. 6, 2021, 3:40 a.m.
This appears common in both the if and the else conditions. You should be able to just do it once …	chipx86	Dec. 6, 2021, 3:40 a.m.
Do we need the .filter if we're already looping and checking values? Might as well just loop once, right?	chipx86	Dec. 6, 2021, 3:40 a.m.
These can be const.	chipx86	Dec. 6, 2021, 3:40 a.m.
Every time there's a comment after a statement, separate them with a blank line. This applies to any other relevant …	chipx86	Dec. 6, 2021, 3:40 a.m.
Multi-line comments must always be in the form of: /* * ... * ... */ Also, make sure all comments …	chipx86	Dec. 6, 2021, 3:40 a.m.
These can be const.	chipx86	Dec. 6, 2021, 3:40 a.m.
These can be const.	chipx86	Dec. 6, 2021, 3:40 a.m.
There should be a space before/after operators. This applies to other lines below.	chipx86	Dec. 6, 2021, 3:40 a.m.
These can be const.	chipx86	Dec. 6, 2021, 3:40 a.m.
This will need to have a key/value pair per line, starting after the variable declaration, as demonstrated in a comment …	chipx86	Dec. 6, 2021, 3:40 a.m.
We already pulled out this.viewBox.width and .height into w and h, so let's reuse those, save an attribute lookup.	chipx86	Dec. 6, 2021, 3:40 a.m.
Can you separate the variable declarations from the modifications? Think of code like paragraphs in a book. What is each …	chipx86	Dec. 6, 2021, 3:40 a.m.
Blank line between these.	chipx86	Dec. 6, 2021, 3:40 a.m.
Whitespace on either side of operators. Check the rest of the change for whitespace around operators.	chipx86	Dec. 6, 2021, 3:40 a.m.
We're calling this.svg.getScreenCTM().inverse() more than once. Let's pull that into a variable and reuse it.	chipx86	Dec. 6, 2021, 3:40 a.m.
We're already fetching these above (though the height is using the wrong function!). Let's reuse what we've already fetched.	chipx86	Dec. 6, 2021, 3:40 a.m.
"the" viewBox of "the" SVG. There are other comments that will also need to be updated.	chipx86	Dec. 6, 2021, 3:40 a.m.
This isn't really describing what this is. A lot of your other functions are giving the reader enough information to …	chipx86	Dec. 6, 2021, 3:40 a.m.
This is indented too far.	chipx86	Dec. 6, 2021, 3:40 a.m.
This will need a trailing comma.	chipx86	Dec. 6, 2021, 3:40 a.m.
Can you move these up by the other click events, in alphabetical order?	chipx86	Dec. 6, 2021, 3:40 a.m.
This needs a trailing comma.	chipx86	Dec. 6, 2021, 3:40 a.m.
The parameters are no longer aligned.	chipx86	Dec. 6, 2021, 3:40 a.m.
Going back to paragraphs: We're adjusting positions once the SVG has loaded. We're setting state for the view. We're managing …	chipx86	Dec. 6, 2021, 3:40 a.m.
This can use dedent (grep the codebase for usage.)	chipx86	Dec. 6, 2021, 3:40 a.m.
The class names used in this block of HTML are very generic. Not your fault — that's pretty common in …	chipx86	Dec. 6, 2021, 3:40 a.m.
Our UI for image review isn't very accessible-friendly (ignore for a moment that the concept of reviewing a visual medium …	chipx86	Dec. 6, 2021, 3:40 a.m.
Paragraphs: We're notifying anyone interested that the viewbox has changed. We're updating comment blocks. I'm not sure that this code …	chipx86	Dec. 6, 2021, 3:40 a.m.
"trigger a corresponding handler".	chipx86	Dec. 6, 2021, 3:40 a.m.
Better to have conditionals prioritize the truthy value when there's an else involved. So instead: if (this.enableCommenting) { ... } …	chipx86	Dec. 6, 2021, 3:40 a.m.
"trigger a corresponding handler" Same with some other comments below.	chipx86	Dec. 6, 2021, 3:40 a.m.
Same note as above regarding the truthy conditionals first. Applies to others below.	chipx86	Dec. 6, 2021, 3:40 a.m.
This is something I feel the SVG implementation should be thinking about.	chipx86	Dec. 6, 2021, 3:40 a.m.
The chaining needs to be indented 4 spaces relative to this.$el.	chipx86	Dec. 6, 2021, 3:40 a.m.
Let's avoid the repeated this.model and just pull it out into a local variable first.	chipx86	Dec. 6, 2021, 3:40 a.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 1)
Show all issues
```
Col: 44
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 1)
Show all issues
```
Col: 30
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 1)
Show all issues
```
Col: 44
 ['width'] is better written in dot notation.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 1)
Show all issues
```
Col: 44
 ['height'] is better written in dot notation.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 1)
Show all issues
```
Col: 43
 ['width'] is better written in dot notation.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 1)
Show all issues
```
Col: 44
 ['height'] is better written in dot notation.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 1)
Show all issues
```
Col: 15
 '$image' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 1)
Show all issues
```
Col: 63
 Expected '===' and instead saw '=='.
```

Added Files:

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com

Diff:

Revision 2 (+329 -15)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com

Diff:

Revision 3 (+329 -23)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

This event is getting ignored completely and I am not super sure why. The only reasoning I could find was that the event does not get triggered if the views are nested.

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

Show all issues

This event works. I have tried this because I wanted to see if mousewheel event works at all. So my best guess (from another comment), is that events get cancelled out when views are rendered in nested approach.

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

Show all issues

This is listening for events on an element with the image-diff-attachment which is a child of the current view, but it looks like this view is attached to an <img> with no children. You probably want to just change the key to be mousewheel.

akim.ruslanov Oct. 16, 2021, 5:17 p.m.

So I tried that, but the same issue persists...

akim.ruslanov Oct. 16, 2021, 5:58 p.m.

So after some more research, I found out that the events in subviews might be killed during the re-rendering of the views or swapping them out. I tried using this._imageView.delegateEvents() but it didn't help either. Perhaps, I was misusing this method or am I misunderstanding the reason why the event is getting triggered?

Change Summary:

Event handling for mousewheel is now handled in
parent view and then relayed to child view.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com

Diff:

Revision 4 (+377 -57)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 4)
Show all issues
```
Col: 57
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 4)
Show all issues
```
Col: 13
 'imageSrc' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 4)
Show all issues
```
Col: 39
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 4)
Show all issues
```
Col: 43
 Missing semicolon.
```

Change Summary:

Fixed zooming in offset caused due to

selection area. Moved from listening to

the changes in viewBox, to directly trigerring

function in child view from parent view

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com

Diff:

Revision 5 (+369 -115)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 5)
Show all issues
```
Col: 23
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 5)
Show all issues
```
Col: 14
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 5)
Show all issues
```
Col: 11
 Missing semicolon.
```

Change Summary:

Fixed zooming in offset caused due to

selection area. Moved from listening to

the changes in viewBox, to directly trigerring

function in child view from parent view.

Also fixed jshint problems.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com

Diff:

Revision 6 (+372 -118)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

This function's sole purpose is to change viewBox value for the SVG based on the values passed from the parent view. One thing I am struggling to understand is to how correctly change the src value of the image. The current implementation is based on loadImages function. This works perfectly fine but I can't explain completely why.
The part I am most confused about is why does line 334 trigger image.onload.

david Nov. 2, 2021, 1:06 p.m.

Changing the src attribute makes the browser "load" a new image, even if it's technically the same source file as before. One of the things about SVG is that the entire structure of the image is accessible via the DOM, so I'd look into drilling down into the element to see if there's a different way to change these attributes without completely reloading the image.

akim.ruslanov Nov. 3, 2021, 11:25 p.m.

So I think I found a way to change the viewBox without reloading the image. However, this eiter requires using invline SVG or using iframe/object tag. I am not super sure if inlining is possible due to caching issues, while iframe/object tags requires changing web configs. I am not sure if it's worth doing since the current implementation seems to be working properly for now.

akim.ruslanov Nov. 4, 2021, 12:11 a.m.

Actually, I kinda figured it out. I can change the src using $el.attr(). I am pretty sure it's not reloading the image every single time anymore. Will post updated code asap

akim.ruslanov Nov. 4, 2021, 12:27 a.m.

NVM, found out that this also triggers reloading... So still kinda stuck

Change Summary:

Added a GIF of how the current
implmentation looks like

Added Files:

Change Summary:

Added a GIF showing a problem that occurs
with commenting and zoom in/out

Added Files:

Problem with zoom in and commenting (Revision 1)

Show all issues

Is this an expected behaviour, or should the comment also change its position based on the zoom in/out of SVG?

akim.ruslanov Nov. 2, 2021, 12:28 a.m.

Just realized that on the comment, you cannot see it, so please open the actual GIF

david Nov. 2, 2021, 12:40 p.m.

We should definitely be scaling the comment outlines when scaling the image.

Change Summary:

Added a button to switch between spanning and commenting.
Added an ability to drag SVG using mouse

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com

Diff:

Revision 7 (+819 -287)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 7)
Show all issues
```
Col: 19
 '$image' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 7)
Show all issues
```
Col: 14
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 7)
Show all issues
```
Col: 23
 '$image' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 7)
Show all issues
```
Col: 18
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 7)
Show all issues
```
Col: 23
 '$image' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 7)
Show all issues
```
Col: 18
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 7)
Show all issues
```
Col: 38
 Missing semicolon.
```

Change Summary:

Dragging after the SVG was zoomed in no
longer triggers the SVG to be in the
original position. Comments scale with SVG
but the problem of position when zooming in
still presists.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com

Diff:

Revision 8 (+879 -309)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 19
 '$image' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 14
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 23
 '$image' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 18
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 23
 '$image' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 18
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 59
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 61
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 54
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 8)
Show all issues
```
Col: 38
 Missing semicolon.
```

Change Summary:

Fixed multiple JSHint issues

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com

Diff:

Revision 9 (+835 -365)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

As far as I understand, the main purpose of loadImages and onImagesLoaded is to ensure data like initial-width and scaling is correct on render.

What I am trying to do right now is to dynamically inline SVGs by fetching the data and parsing it using the DOMParser. This will allow me to use SVGElement API that's somewhat necessary for correct scaling, dragging and zooming. Should I be dynamically inlining the SVGs in loadImages or should that be done before any rendering even begins?

Change Summary:

Possibly need to inline SVG to use
the SVGElement API. Trying to figure
out if this is possible

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com

Diff:

Revision 10 (+977 -381)

Show changes

reviewboard/static/rb/js/views/imageReviewableView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 10)
Show all issues
```
Col: 11
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 10)
Show all issues
```
Col: 11
 Missing semicolon.
```

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

Show all issues

In order to inline the SVG, I need to asynchronously fetch them and use the parser after. However, if I do it like this, then the renderring of the image happens couple of seconds after the rest of the page got rendered. In addition, since we run some methods on the element, they fail since it didn't finish rendering. Any thoughts on how to combat that? I was thinking of maybe fetching the data in backend, or perhaps there is a way to fetch it in the model?

chipx86 Nov. 22, 2021, 6:26 p.m.

It'll need to happen on the frontend. Maybe put up a spinner before fetching, remove it after fetching? I could see this being an issue for more than just SVGs down the road, so having a way to say "I'm triggering a load of the image" and then "I got it, we can set things up now" would be good.

akim.ruslanov Nov. 22, 2021, 6:31 p.m.
```
OK sounds good!
```

akim.ruslanov Nov. 22, 2021, 6:33 p.m.

Also, as a quick follow up, does any view/element of RB have something that has a spinner already? Just wanna make sure I am following the same UI pattern as everything before

chipx86 Nov. 22, 2021, 8:55 p.m.

Yep. We do have an older spinner style that you'll find, but grep around for djblets-o-spinner and you'll see the modern one we're migrating over to.

Change Summary:

Finished migrating to SVGElements API.
Now any scaling, zooming or dragging happen
by changing the viewBox value directly.
The buttons are more flushed out.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com

Diff:

Revision 11 (+1378 -496)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 44
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 38
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 46
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 11
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 60
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 62
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 92
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 130
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 150
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 15
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 11
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 11)
Show all issues
```
Col: 72
 Missing semicolon.
```

Change Summary:

Finished migrating to SVGElements API.
Now any scaling, zooming or dragging happen
by changing the viewBox value directly.
The buttons are more flushed out.
Fixed JSHint issues

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com

Diff:

Revision 12 (+1383 -507)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 12)
Show all issues
```
Col: 46
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 12)
Show all issues
```
Col: 72
 Missing semicolon.
```

Change Summary:

Finished migrating to SVGElements API.
Now any scaling, zooming or dragging happen
by changing the viewBox value directly.
The buttons are more flushed out.
Fixed JSHint issues

Removed Files:
Added Files:

Change Summary:

Added documentation to all new methods and
views. Code is cleaned up according to the
previous patterns

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com

Diff:

Revision 13 (+1676 -602)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 13)
Show all issues
```
Col: 46
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 13)
Show all issues
```
Col: 40
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 13)
Show all issues
```
Col: 72
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 13)
Show all issues
```
Col: 19
 'offset' is defined but never used.
```

Change Summary:

Fixed whitespaces. Added a new method to
stringify viewBox so code wraps nicely in
review.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
-	added documentation; cleaned up code	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com
+	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com

Diff:

Revision 14 (+1728 -626)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 7
 'SvgAttachmentView' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 46
 Missing semicolon.
```

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

Show all issues

Col: 5
 Expected '}' to match '{' from line 275 and instead saw 'getSelectionRegion'.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 23
 Unorthodox function invocation.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 26
 Expected ')' and instead saw '{'.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 27
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 1
 Unexpected '}'.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 1
 Expected ')' to match '(' from line 1 and instead saw '}'.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 1
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 2
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 2
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 2
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 14)
Show all issues
```
Col: 72
 Missing semicolon.
```

Change Summary:

Fixed bug with stringifyViewBox where
it failed in the cases of cached viewBox

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
-	added documentation; cleaned up code	aruslanov@fispan.com
-	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com
+	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
+	fixed more whitespaces	aruslanov@fispan.com
+	small bug with new method	aruslanov@fispan.com
+	mend	aruslanov@fispan.com

Diff:

Revision 15 (+1747 -637)

Show changes

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

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Col: 46
 Missing semicolon.
```
1. akim.ruslanov Dec. 1, 2021, 7:03 p.m.
  Promise is chained here, so putting semicolon breaks it.
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Col: 72
 Missing semicolon.
```
1. akim.ruslanov Dec. 1, 2021, 7:03 p.m.
  There is a semicolon present. Not exactly sure what is the problem.

Description:

		Currently ReviewBoard has a limited way of rendering SVG files and does
		not allow for proper scaling and zooming. We are now attempting to utilize
		SVG potential to full extent and allow users to inifinitely scale/zoom SVG images.

		The new solution utilizes `viewBox` attribute of SVGs and attempts to manipulate it
~		in order to create zoom-in ability.
	~	in order to create zoom-in ability and ability to span SVG.

Summary:

-	WIP: Review UI for SVG Vector Graphics files
+	Review UI for SVG Vector Graphics files

Fix it!

reviewboard/static/rb/css/pages/image-review-ui.less (Diff revision 15)

Show all issues

Please change #444 to #rb-ns-ui.colors[@grey-20]

akim.ruslanov Dec. 1, 2021, 6:55 p.m.

Fixed. Also fixed old components using #444 instead of #rb-ns-ui.colors[@grey-20]

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
This should probably be set in initialize instead of here.
```

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

Show all issues

Can we call this svgPromise, just so people definitely know that it's a promise and not data?

akim.ruslanov Dec. 1, 2021, 6:55 p.m.

Changed the variable to this.svgPromise. Should the name of the method also change, or is fetchSvg() clear enough? In my mind, fetch is always async (unless explicitly said otherwise) so any fetch type function will return a Promise

chipx86 Dec. 6, 2021, 2:56 a.m.

I think fetchSvg() is fine. It's more about identifying the type of the variable, to make it clear.

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

Show all issues

This looks like it's not actually a jQuery object, so we shouldn't use the $ in the variable name.

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

Show all issues

Let's split this up:

this.svgSize = {
    width: this.viewBox.width,
    height: this.viewBox.height,
};

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

Show all issues

fetch already is a promise, so this whole thing can just be:

return fetch(this.model.get('imageURL'))
    .then(res => res.text())
    .then(data => {
        const parser = new DOMParser();
        return parser.parseFromString(data, 'image/svg+xml')
            .querySelector('svg');
    });

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
What's going on here?
```
1. akim.ruslanov Dec. 1, 2021, 6:55 p.m.
  Typo, my bad!
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Add a blank line between these.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Too much indentation in here.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Add a blank line between these.
```

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

Show all issues

Can you explain this? Is this a TODO or FIXME? (if so, please use that text so it's searchable). This comment should also probably go inside the body of the function instead of above it.

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

Show all issues

This needs some blank lines to make it less dense. Comments also need to start with capital letters and end with periods.

akim.ruslanov Dec. 1, 2021, 6:55 p.m.

Added some blank lines in the most recent commit. Do you think it is more readable now? Tried to separate everything based on what math is happening in each block.

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

Show all issues

Indent as such:

this.viewBox = {
      x: ...,
      y: ...,
};

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Add a trailing comma here.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Add spaces around - operator.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Line is too long. Please wrap to 80 columns throughout.
```

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

Show all issues

svgFetched is already a promise, so we don't need to wrap with return new Promise(() => {})

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Typo?
```

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

Show all issues

This shouldn't need async

akim.ruslanov Dec. 1, 2021, 6:55 p.m.

Since getSelectionRegion() is async for the SvgAttachmentView, I am using await for it. That's why I put async there

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

Show all issues

Did you test this with normal image attachments?

akim.ruslanov Dec. 1, 2021, 6:55 p.m.

Yes. In the case with normal image, this._imageView.getSelectionRegion is synchronous, so await does not really do anything here. So this whole code block just behaves synchronously

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
We can use this.model.get('filename').endsWith('.svg')
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
This needs a trailing comma.
```

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

Show all issues

Parens and {} aren't necessary in here. Can just be .then(res => this._adjustPos())

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

Show all issues

If you search the JS code for dedent you'll see a better way to write this literal.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Add a blank line between these two.
```

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

Show all issues

This can use addClass. Also, please use single quotes:
this.$('#dragging-mode-icon').addClass('fa fa-arrows')

akim.ruslanov Dec. 1, 2021, 6:55 p.m.

In this case, I am trying to toggle the icon by changing the class, that's why I am using attr. If I change it to addClass it will no longer toggle the icons. Or am I misunderstanding your comment?

chipx86 Dec. 6, 2021, 2:56 a.m.

I have some comments coming up that will make this moot.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Same here.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
Add a blank line between these two.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
And here.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
And here.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
There's an extra space character at the end of this line.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
This new blank line should be removed.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
This line is definitely too long. Please wrap to 80 columns throughout.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
This new blank line should be removed.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
Show all issues
```
This new blank line should be removed.
```
1. akim.ruslanov Dec. 1, 2021, 6:55 p.m.
  I don't think that's a blank line

Change Summary:

Made changes according to David's comments.
Also wrapped other documentation in a better
way.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
-	added documentation; cleaned up code	aruslanov@fispan.com
-	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
-	fixed more whitespaces	aruslanov@fispan.com
-	small bug with new method	aruslanov@fispan.com
-	mend	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com
+	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
+	fixed more whitespaces	aruslanov@fispan.com
+	small bug with new method	aruslanov@fispan.com
+	mend	aruslanov@fispan.com
+	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com

Diff:

Revision 16 (+1886 -748)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 16)
Show all issues
```
Col: 66
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 16)
Show all issues
```
Col: 32
 Missing semicolon.
```

Change Summary:

Made changes according to David's comments.
Also wrapped other documentation in a better
way.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
-	added documentation; cleaned up code	aruslanov@fispan.com
-	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
-	fixed more whitespaces	aruslanov@fispan.com
-	small bug with new method	aruslanov@fispan.com
-	mend	aruslanov@fispan.com
-	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com
+	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
+	fixed more whitespaces	aruslanov@fispan.com
+	small bug with new method	aruslanov@fispan.com
+	mend	aruslanov@fispan.com
+	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
+	missed out on some small whitespaces and semicolons	aruslanov@fispan.com

Diff:

Revision 17 (+1886 -750)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Comment boxes are scaled dynamically with
SVGs.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
-	added documentation; cleaned up code	aruslanov@fispan.com
-	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
-	fixed more whitespaces	aruslanov@fispan.com
-	small bug with new method	aruslanov@fispan.com
-	mend	aruslanov@fispan.com
-	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
-	missed out on some small whitespaces and semicolons	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com
+	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
+	fixed more whitespaces	aruslanov@fispan.com
+	small bug with new method	aruslanov@fispan.com
+	mend	aruslanov@fispan.com
+	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
+	missed out on some small whitespaces and semicolons	aruslanov@fispan.com
+	comments are scaled with SVGs	aruslanov@fispan.com

Diff:

Revision 18 (+2025 -765)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 18)
Show all issues
```
Col: 19
 'offset' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 18)
Show all issues
```
Col: 80
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 18)
Show all issues
```
Col: 19
 'offset' is defined but never used.
```

Change Summary:

Comment boxes are dynamically scaled
and moved based on the viewBox. Whitespaces
and indentation fixed.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
-	added documentation; cleaned up code	aruslanov@fispan.com
-	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
-	fixed more whitespaces	aruslanov@fispan.com
-	small bug with new method	aruslanov@fispan.com
-	mend	aruslanov@fispan.com
-	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
-	missed out on some small whitespaces and semicolons	aruslanov@fispan.com
-	comments are scaled with SVGs	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com
+	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
+	fixed more whitespaces	aruslanov@fispan.com
+	small bug with new method	aruslanov@fispan.com
+	mend	aruslanov@fispan.com
+	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
+	missed out on some small whitespaces and semicolons	aruslanov@fispan.com
+	comments are scaled with SVGs	aruslanov@fispan.com
+	fixing whitespaces	aruslanov@fispan.com

Diff:

Revision 19 (+2028 -768)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 19)
Show all issues
```
Col: 19
 'offset' is defined but never used.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 19)
Show all issues
```
Col: 80
 Missing semicolon.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 19)
Show all issues
```
Col: 19
 'offset' is defined but never used.
```

Change Summary:

Comment boxes are dynamically scaled
and moved based on the viewBox. Whitespaces
and indentation fixed.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
-	added documentation; cleaned up code	aruslanov@fispan.com
-	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
-	fixed more whitespaces	aruslanov@fispan.com
-	small bug with new method	aruslanov@fispan.com
-	mend	aruslanov@fispan.com
-	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
-	missed out on some small whitespaces and semicolons	aruslanov@fispan.com
-	comments are scaled with SVGs	aruslanov@fispan.com
-	fixing whitespaces	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com
+	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
+	fixed more whitespaces	aruslanov@fispan.com
+	small bug with new method	aruslanov@fispan.com
+	mend	aruslanov@fispan.com
+	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
+	missed out on some small whitespaces and semicolons	aruslanov@fispan.com
+	comments are scaled with SVGs	aruslanov@fispan.com
+	fixing whitespaces	aruslanov@fispan.com
+	jshint issues fixed	aruslanov@fispan.com

Diff:

Revision 20 (+2030 -774)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Updated images with explanations of buttons.

Removed Files:
Added Files:

Change Summary:

Updated description and testing.

Description:

		Currently ReviewBoard has a limited way of rendering SVG files and does
		not allow for proper scaling and zooming. We are now attempting to utilize
		SVG potential to full extent and allow users to inifinitely scale/zoom SVG images.

		The new solution utilizes `viewBox` attribute of SVGs and attempts to manipulate it
~		in order to create zoom-in ability and ability to span SVG.
	~	in order to create zoom-in ability and ability to span SVG. Moreover, the solution
	+	also dynamically rescales and adjusts comment boxes based on the `viewBox`.
	+
	+	Currently, the new features only apply to a single image, i.e. no diffs view.
	+	The diff view with SVGs will behave the same way as with the bitmap images.
	+	However, there is no ability to zoom in/out or span the SVG.

Testing Done:

		The UI was tested using Google Chrome and Safari.

~		No unit tests were run yet and no additional tests added.
	~	Unit tests were but no additional tests were added.

Change Summary:

Fixing whitespaces.

Commits:

	Summary	Author
-	wip initial	aruslanov@fispan.com
-	mousewheel works in parent view	aruslanov@fispan.com
-	clean up	aruslanov@fispan.com
-	fixed event handling	aruslanov@fispan.com
-	onload question	aruslanov@fispan.com
-	fixed jshint problems	aruslanov@fispan.com
-	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
-	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
-	fixed jshint issues	aruslanov@fispan.com
-	figuring out fetching within render	aruslanov@fispan.com
-	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
-	fixing some jshint issues	aruslanov@fispan.com
-	added documentation; cleaned up code	aruslanov@fispan.com
-	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
-	fixed more whitespaces	aruslanov@fispan.com
-	small bug with new method	aruslanov@fispan.com
-	mend	aruslanov@fispan.com
-	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
-	missed out on some small whitespaces and semicolons	aruslanov@fispan.com
-	comments are scaled with SVGs	aruslanov@fispan.com
-	fixing whitespaces	aruslanov@fispan.com
-	jshint issues fixed	aruslanov@fispan.com
+	wip initial	aruslanov@fispan.com
+	mousewheel works in parent view	aruslanov@fispan.com
+	clean up	aruslanov@fispan.com
+	fixed event handling	aruslanov@fispan.com
+	onload question	aruslanov@fispan.com
+	fixed jshint problems	aruslanov@fispan.com
+	dragging ability added; new button to switch between spanning and commenting added	aruslanov@fispan.com
+	fixed dragging issue; reviewing the bug with scaling and dragging	aruslanov@fispan.com
+	fixed jshint issues	aruslanov@fispan.com
+	figuring out fetching within render	aruslanov@fispan.com
+	finished transferring to SVGElementsAPI; buttons flushed out	aruslanov@fispan.com
+	fixing some jshint issues	aruslanov@fispan.com
+	added documentation; cleaned up code	aruslanov@fispan.com
+	fixed whitespace; added a method to stringify viewBox	aruslanov@fispan.com
+	fixed more whitespaces	aruslanov@fispan.com
+	small bug with new method	aruslanov@fispan.com
+	mend	aruslanov@fispan.com
+	Changes according to David's comments; made sure everything wraps nicely with 80 chars	aruslanov@fispan.com
+	missed out on some small whitespaces and semicolons	aruslanov@fispan.com
+	comments are scaled with SVGs	aruslanov@fispan.com
+	fixing whitespaces	aruslanov@fispan.com
+	jshint issues fixed	aruslanov@fispan.com
+	i hate whitespaces	aruslanov@fispan.com

Diff:

Revision 21 (+2032 -776)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

This feature is great! Now, I have a lot of comments here. Most of them have to do with coding style standards (trailing commas, blank lines, indentation, ordering, etc.). The kind of things that make a review large, but will help the code really fit and stay maintainable once complete.

There is a bigger note in here, and I'd take it on after the stylistic stuff so that those bits are less distracting. That note has to do with the UI. I think what you had was great for development, but buttons that toggle their behavior when clicked is problematic, for reasons I'll outline below.

Show all issues

The new toolbar seems to be SVG-specific. It would be ideal if the ability to zoom, pan, etc. could be done on bitmap as well. Is that possible? If not, what would be required to make that happen (as work for after this goes in)?

Ideally, we'd end up with one unifying interface that was common regardless of image type.

UI elements to control SVG (Revision 3)

Show all issues

In UI, discoverability is important. A person should ideally be able to see what options are available before having to interact. In this case, we have two icons that each conceal an alternate state, with no way (aside from trial-and-error or reading documentation) to know about that state.

I'd suggest splitting each of these into two buttons (with some separation between them to help show the grouping), and use a visual indicator to show which is selected (such as a background color that makes it clear which is active).

That way, a person will see their options up-front, and can choose them without having to remember what happens when they click each thing.

This is also really important for accessibility. If a person is using a screen reader, they'll have an even more difficult time selecting the desired action.

reviewboard/static/rb/css/pages/image-review-ui.less (Diff revision 21)

Show all issues

Can you place this so that it's listed alphabetically with other classes?

reviewboard/static/rb/css/pages/image-review-ui.less (Diff revision 21)
Show all issues
```
There's a blank line here that you can get rid of.
```

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

Show all issues

Since we have svg as a local variable, use it here and everywhere else in this function instead of this.svg (which requires an attribute lookup).

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
Comments should always end in a period.
```

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

Show all issues

When building an object, format it like:

this.svgSize = {
    width: this.viewBox.width,
    height: this.viewBox.height,
};

(Notice the trailing comma on the last line as well. This avoids needing to change that line if ever adding more entries for any reason.)

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

Show all issues

This appears common in both the if and the else conditions. You should be able to just do it once after the conditionals end.

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

Show all issues

Do we need the .filter if we're already looping and checking values? Might as well just loop once, right?

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
These can be const.
```

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

Show all issues

Every time there's a comment after a statement, separate them with a blank line.

This applies to any other relevant comments in the change.

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

Show all issues

Multi-line comments must always be in the form of:

/*
 * ...
 * ...
 */

Also, make sure all comments are in sentence casing with trailing periods.

This applies to any other relevant comments in the change.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
These can be const.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
These can be const.
```

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

Show all issues

There should be a space before/after operators.

This applies to other lines below.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
These can be const.
```

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

Show all issues

This will need to have a key/value pair per line, starting after the variable declaration, as demonstrated in a comment above.

This applies to other such objects below.

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

Show all issues

We already pulled out this.viewBox.width and .height into w and h, so let's reuse those, save an attribute lookup.

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

Show all issues

Can you separate the variable declarations from the modifications?

Think of code like paragraphs in a book. What is each paragraph trying to communicate? Is it a big dense paragraph that's trying to say many things, or does each have purpose?

I see the following paragraphs in this function:

We're determining the scaling based on stored state.
We're telling the element what its size should be.
We're telling the SVG what its new size and viewBox should be.
We're setting new state on the iew.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
Blank line between these.
```

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

Show all issues

Whitespace on either side of operators.

Check the rest of the change for whitespace around operators.

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

Show all issues

We're calling this.svg.getScreenCTM().inverse() more than once. Let's pull that into a variable and reuse it.

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

Show all issues

We're already fetching these above (though the height is using the wrong function!).

Let's reuse what we've already fetched.

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

Show all issues

"the" viewBox of "the" SVG.

There are other comments that will also need to be updated.

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

Show all issues

This isn't really describing what this is. A lot of your other functions are giving the reader enough information to know what to expect, and this should follow that pattern.

Also, make sure you aren't leaving off periods at the end of sentences.

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

Show all issues

This is indented too far.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
This will need a trailing comma.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
Can you move these up by the other click events, in alphabetical order?
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
This needs a trailing comma.
```
reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
The parameters are no longer aligned.
```

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

Show all issues

Going back to paragraphs:

We're adjusting positions once the SVG has loaded.
We're setting state for the view.
We're managing the DOM to display the view.

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

Show all issues

This can use dedent (grep the codebase for usage.)

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

Show all issues

The class names used in this block of HTML are very generic. Not your fault — that's pretty common in a lot of our legacy code. We're trying to fix this with our CSS Component style: https://reviewboard.notion.site/CSS-LessCSS-Component-Style-Guide-ceb55ed87d954101a074f6601fc5deb2
You're not going to be able to retrofit all this onto that. No problem. Let's namespace these classes, though, and call them:

rb-c-image-review-svg-toolbar
rb-c-image-review-svg-toolbar__pan
rb-c-image-review-svg-toolbar__zoom
rb-c-image-review-svg-toolbar__drag
rb-c-image-review-svg-toolbar__comment

(This is based on prior suggestions on separating these.)

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

Show all issues

Our UI for image review isn't very accessible-friendly (ignore for a moment that the concept of reviewing a visual medium sort of isn't either).

Still, we want to make forward progress on any new UI. These actions need to be presentable to screen readers or any other accessibility tools. We can do that by:

Telling the screen reader these are effectively buttons (role="button")
Giving them a title/tooltip (title="...") — basically what you'd want to see as alternative text for the icon, if the icon could not be shown.
Turn off reading of the icon (aria-hidden="true")

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

Show all issues

Paragraphs:

We're notifying anyone interested that the viewbox has changed.
We're updating comment blocks.

I'm not sure that this code should be assuming SVGs or viewboxes, though. I think it should be thinking in terms of zooming. Let the specific implementation know what that means.

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
"trigger a corresponding handler".
```

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

Show all issues

Better to have conditionals prioritize the truthy value when there's an else involved. So instead:

if (this.enableCommenting) {
    ...
} else {
    ...
}

reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
Show all issues
```
"trigger a corresponding handler"
Same with some other comments below.
```

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

Show all issues

Same note as above regarding the truthy conditionals first.

Applies to others below.

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

Show all issues

This is something I feel the SVG implementation should be thinking about.

reviewboard/static/rb/js/views/regionCommentBlockView.es6.js (Diff revision 21)

Show all issues

The chaining needs to be indented 4 spaces relative to this.$el.

reviewboard/static/rb/js/views/regionCommentBlockView.es6.js (Diff revision 21)

Show all issues

Let's avoid the repeated this.model and just pull it out into a local variable first.

Review UI for SVG Vector Graphics files

JSHint

Added Files:

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Added Files:

Change Summary:

Added Files:

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Removed Files:

Added Files:

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commits:

Diff:

Added Files:

Checks run (1 failed, 1 succeeded)

JSHint

Description:

Summary:

Change Summary:

Commits:

Diff: