Review UI for SVG Vector Graphics files

Review Request #11846 — Created Oct. 8, 2021 and updated

akim.ruslanov
Review Board
release-4.0.x
students

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.

The UI was tested using Google Chrome and Safari.

Unit tests were but no additional tests were added.

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

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 ...

chipx86chipx86

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

akim.ruslanovakim.ruslanov

In UI, discoverability is important. A person should ideally be able to see what options are available before having to ...

chipx86chipx86

Col: 44 Missing semicolon.

reviewbotreviewbot

Col: 30 Missing semicolon.

reviewbotreviewbot

Col: 44 ['width'] is better written in dot notation.

reviewbotreviewbot

Col: 44 ['height'] is better written in dot notation.

reviewbotreviewbot

Col: 43 ['width'] is better written in dot notation.

reviewbotreviewbot

Col: 44 ['height'] is better written in dot notation.

reviewbotreviewbot

Col: 15 '$image' is defined but never used.

reviewbotreviewbot

Col: 63 Expected '===' and instead saw '=='.

reviewbotreviewbot

This is listening for events on an element with the image-diff-attachment which is a child of the current view, but ...

daviddavid

This event is getting ignored completely and I am not super sure why. The only reasoning I could find was ...

akim.ruslanovakim.ruslanov

This event works. I have tried this because I wanted to see if mousewheel event works at all. So my ...

akim.ruslanovakim.ruslanov

Col: 57 Missing semicolon.

reviewbotreviewbot

Col: 13 'imageSrc' is defined but never used.

reviewbotreviewbot

Col: 39 Missing semicolon.

reviewbotreviewbot

Col: 43 Missing semicolon.

reviewbotreviewbot

Col: 23 Missing semicolon.

reviewbotreviewbot

Col: 14 Missing semicolon.

reviewbotreviewbot

Col: 11 Missing semicolon.

reviewbotreviewbot

This function's sole purpose is to change viewBox value for the SVG based on the values passed from the parent ...

akim.ruslanovakim.ruslanov

Col: 19 '$image' is defined but never used.

reviewbotreviewbot

Col: 14 Missing semicolon.

reviewbotreviewbot

Col: 23 '$image' is defined but never used.

reviewbotreviewbot

Col: 18 Missing semicolon.

reviewbotreviewbot

Col: 23 '$image' is defined but never used.

reviewbotreviewbot

Col: 18 Missing semicolon.

reviewbotreviewbot

Col: 38 Missing semicolon.

reviewbotreviewbot

Col: 19 '$image' is defined but never used.

reviewbotreviewbot

Col: 14 Missing semicolon.

reviewbotreviewbot

Col: 23 '$image' is defined but never used.

reviewbotreviewbot

Col: 18 Missing semicolon.

reviewbotreviewbot

Col: 23 '$image' is defined but never used.

reviewbotreviewbot

Col: 18 Missing semicolon.

reviewbotreviewbot

Col: 59 Missing semicolon.

reviewbotreviewbot

Col: 61 Missing semicolon.

reviewbotreviewbot

Col: 54 Missing semicolon.

reviewbotreviewbot

Col: 38 Missing semicolon.

reviewbotreviewbot

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

akim.ruslanovakim.ruslanov

In order to inline the SVG, I need to asynchronously fetch them and use the parser after. However, if I ...

akim.ruslanovakim.ruslanov

Col: 11 Missing semicolon.

reviewbotreviewbot

Col: 11 Missing semicolon.

reviewbotreviewbot

Col: 44 Missing semicolon.

reviewbotreviewbot

Col: 38 Missing semicolon.

reviewbotreviewbot

Col: 46 Missing semicolon.

reviewbotreviewbot

Col: 11 Missing semicolon.

reviewbotreviewbot

Col: 60 Missing semicolon.

reviewbotreviewbot

Col: 62 Missing semicolon.

reviewbotreviewbot

Col: 92 Missing semicolon.

reviewbotreviewbot

Col: 130 Missing semicolon.

reviewbotreviewbot

Col: 150 Missing semicolon.

reviewbotreviewbot

Col: 15 Missing semicolon.

reviewbotreviewbot

Col: 11 Missing semicolon.

reviewbotreviewbot

Col: 72 Missing semicolon.

reviewbotreviewbot

Col: 46 Missing semicolon.

reviewbotreviewbot

Col: 72 Missing semicolon.

reviewbotreviewbot

Col: 46 Missing semicolon.

reviewbotreviewbot

Col: 40 Missing semicolon.

reviewbotreviewbot

Col: 72 Missing semicolon.

reviewbotreviewbot

Col: 19 'offset' is defined but never used.

reviewbotreviewbot

Col: 7 'SvgAttachmentView' is defined but never used.

reviewbotreviewbot

Col: 46 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 23 Unorthodox function invocation.

reviewbotreviewbot

Col: 26 Expected ')' and instead saw '{'.

reviewbotreviewbot

Col: 27 Missing semicolon.

reviewbotreviewbot

Col: 1 Unexpected '}'.

reviewbotreviewbot

Col: 1 Expected ')' to match '(' from line 1 and instead saw '}'.

reviewbotreviewbot

Col: 1 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2 Missing semicolon.

reviewbotreviewbot

Col: 2 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 2 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 72 Missing semicolon.

reviewbotreviewbot

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

daviddavid

This should probably be set in initialize instead of here.

daviddavid

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

daviddavid

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

daviddavid

Let's split this up: this.svgSize = { width: this.viewBox.width, height: this.viewBox.height, };

daviddavid

fetch already is a promise, so this whole thing can just be: return fetch(this.model.get('imageURL')) .then(res => res.text()) .then(data => { ...

daviddavid

Col: 46 Missing semicolon.

reviewbotreviewbot

What's going on here?

daviddavid

Add a blank line between these.

daviddavid

Too much indentation in here.

daviddavid

Add a blank line between these.

daviddavid

Can you explain this? Is this a TODO or FIXME? (if so, please use that text so it's searchable). This ...

daviddavid

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

daviddavid

Indent as such: this.viewBox = { x: ..., y: ..., };

daviddavid

Add a trailing comma here.

daviddavid

Add spaces around - operator.

daviddavid

Line is too long. Please wrap to 80 columns throughout.

daviddavid

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

daviddavid

Typo?

daviddavid

This shouldn't need async

daviddavid

Did you test this with normal image attachments?

daviddavid

We can use this.model.get('filename').endsWith('.svg')

daviddavid

This needs a trailing comma.

daviddavid

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

daviddavid

Col: 72 Missing semicolon.

reviewbotreviewbot

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

daviddavid

Add a blank line between these two.

daviddavid

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

daviddavid

Same here.

daviddavid

Add a blank line between these two.

daviddavid

And here.

daviddavid

And here.

daviddavid

There's an extra space character at the end of this line.

daviddavid

This new blank line should be removed.

daviddavid

This line is definitely too long. Please wrap to 80 columns throughout.

daviddavid

This new blank line should be removed.

daviddavid

This new blank line should be removed.

daviddavid

Col: 66 Missing semicolon.

reviewbotreviewbot

Col: 32 Missing semicolon.

reviewbotreviewbot

Col: 19 'offset' is defined but never used.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 19 'offset' is defined but never used.

reviewbotreviewbot

Col: 19 'offset' is defined but never used.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 19 'offset' is defined but never used.

reviewbotreviewbot

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

chipx86chipx86

There's a blank line here that you can get rid of.

chipx86chipx86

Since we have svg as a local variable, use it here and everywhere else in this function instead of this.svg ...

chipx86chipx86

Comments should always end in a period.

chipx86chipx86

When building an object, format it like: this.svgSize = { width: this.viewBox.width, height: this.viewBox.height, }; (Notice the trailing comma on ...

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

These can be const.

chipx86chipx86

Every time there's a comment after a statement, separate them with a blank line. This applies to any other relevant ...

chipx86chipx86

Multi-line comments must always be in the form of: /* * ... * ... */ Also, make sure all comments ...

chipx86chipx86

These can be const.

chipx86chipx86

These can be const.

chipx86chipx86

There should be a space before/after operators. This applies to other lines below.

chipx86chipx86

These can be const.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Can you separate the variable declarations from the modifications? Think of code like paragraphs in a book. What is each ...

chipx86chipx86

Blank line between these.

chipx86chipx86

Whitespace on either side of operators. Check the rest of the change for whitespace around operators.

chipx86chipx86

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

chipx86chipx86

We're already fetching these above (though the height is using the wrong function!). Let's reuse what we've already fetched.

chipx86chipx86

"the" viewBox of "the" SVG. There are other comments that will also need to be updated.

chipx86chipx86

This isn't really describing what this is. A lot of your other functions are giving the reader enough information to ...

chipx86chipx86

This is indented too far.

chipx86chipx86

This will need a trailing comma.

chipx86chipx86

Can you move these up by the other click events, in alphabetical order?

chipx86chipx86

This needs a trailing comma.

chipx86chipx86

The parameters are no longer aligned.

chipx86chipx86

Going back to paragraphs: We're adjusting positions once the SVG has loaded. We're setting state for the view. We're managing ...

chipx86chipx86

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

chipx86chipx86

The class names used in this block of HTML are very generic. Not your fault — that's pretty common in ...

chipx86chipx86

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

chipx86chipx86

Paragraphs: We're notifying anyone interested that the viewbox has changed. We're updating comment blocks. I'm not sure that this code ...

chipx86chipx86

"trigger a corresponding handler".

chipx86chipx86

Better to have conditionals prioritize the truthy value when there's an else involved. So instead: if (this.enableCommenting) { ... } ...

chipx86chipx86

"trigger a corresponding handler" Same with some other comments below.

chipx86chipx86

Same note as above regarding the truthy conditionals first. Applies to others below.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
akim.ruslanov
akim.ruslanov
akim.ruslanov
  1. 
      
  2. reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.

  3. 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.

  4. 
      
david
  1. 
      
  2. 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.

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

    2. 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?

  3. 
      
akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
akim.ruslanov
  1. 
      
  2. reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.

    1. 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.

    2. 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.

    3. 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

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

  3. 
      
akim.ruslanov
akim.ruslanov
akim.ruslanov
  1. 
      
  2. Is this an expected behaviour, or should the comment also change its position based on the zoom in/out of SVG?

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

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

  3. 
      
akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
akim.ruslanov
  1. 
      
  2. reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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?

  3. 
      
akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
  1. 
      
  2. reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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?

    1. 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.

    2. OK sounds good!

    3. 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

    4. 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.

  3. 
      
akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
Review request changed

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

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
akim.ruslanov
david
  1. 
      
  2. Please change #444 to #rb-ns-ui.colors[@grey-20]

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

  3. This should probably be set in initialize instead of here.

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

    1. 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

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

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

  6. Let's split this up:

    this.svgSize = {
        width: this.viewBox.width,
        height: this.viewBox.height,
    };
    
  7. 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');
        });
    
  8. What's going on here?

  9. Add a blank line between these.

  10. Too much indentation in here.

  11. Add a blank line between these.

  12. 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.

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

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

    1. 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.

  14. Indent as such:

    this.viewBox = {
          x: ...,
          y: ...,
    };
    
  15. Add a trailing comma here.

  16. Add spaces around - operator.

  17. Line is too long. Please wrap to 80 columns throughout.

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

  19. This shouldn't need async

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

  20. Did you test this with normal image attachments?

    1. 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

  21. We can use this.model.get('filename').endsWith('.svg')

  22. This needs a trailing comma.

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

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

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

  25. Add a blank line between these two.

  26. This can use addClass. Also, please use single quotes:

    this.$('#dragging-mode-icon').addClass('fa fa-arrows')

    1. 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?

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

  27. Add a blank line between these two.

  28. There's an extra space character at the end of this line.

  29. This new blank line should be removed.

  30. This line is definitely too long. Please wrap to 80 columns throughout.

  31. This new blank line should be removed.

  32. This new blank line should be removed.

    1. I don't think that's a blank line

  33. 
      
akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
akim.ruslanov
akim.ruslanov
akim.ruslanov
Review request changed

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

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
chipx86
  1. 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.

  2. 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.

  3. 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.

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

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

  5. There's a blank line here that you can get rid of.

  6. 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).

  7. Comments should always end in a period.

  8. 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.)

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

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

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

  11. These can be const.

  12. 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.

  13. 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.

  14. These can be const.

  15. These can be const.

  16. There should be a space before/after operators.

    This applies to other lines below.

  17. These can be const.

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

    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.

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

  20. 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:

    1. We're determining the scaling based on stored state.
    2. We're telling the element what its size should be.
    3. We're telling the SVG what its new size and viewBox should be.
    4. We're setting new state on the iew.
  21. Blank line between these.

  22. Whitespace on either side of operators.

    Check the rest of the change for whitespace around operators.

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

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

    Let's reuse what we've already fetched.

  25. "the" viewBox of "the" SVG.

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

  26. 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.

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

    This is indented too far.

  28. This will need a trailing comma.

  29. Can you move these up by the other click events, in alphabetical order?

  30. This needs a trailing comma.

  31. The parameters are no longer aligned.

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

    Going back to paragraphs:

    1. We're adjusting positions once the SVG has loaded.
    2. We're setting state for the view.
    3. We're managing the DOM to display the view.
  33. reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 21)
     
     
     
     
     
     
     
     
     
     
     
     

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

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

    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.)

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

    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:

    1. Telling the screen reader these are effectively buttons (role="button")
    2. 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.
    3. Turn off reading of the icon (aria-hidden="true")
  36. Paragraphs:

    1. We're notifying anyone interested that the viewbox has changed.
    2. 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.

  37. "trigger a corresponding handler".

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

    if (this.enableCommenting) {
        ...
    } else {
        ...
    }
    
  39. "trigger a corresponding handler"

    Same with some other comments below.

  40. Same note as above regarding the truthy conditionals first.

    Applies to others below.

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

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

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

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

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

  44. 
      
Loading...