Review UI for SVG Vector Graphics files

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

Information

Review Board
release-4.0.x

Reviewers

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars
32439633178192f77065896b91aae703ece28e99 aruslanov@fispan.com
missed out on some small whitespaces and semicolons
36ebf8bb1450f9afb7b34ba7e4eba62d934f463a aruslanov@fispan.com
comments are scaled with SVGs
695325381aae48ddf9dc8aa1ce7861c7a8333182 aruslanov@fispan.com
fixing whitespaces
a7bcd730e93043c7ce57d1a96ee746365eb05c60 aruslanov@fispan.com
jshint issues fixed
25b54945aa2ea411da42d7a84e48506aae248d5b aruslanov@fispan.com
i hate whitespaces
94ccf4b1ce3c255929f9a8f2c491a396a60c8243 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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.

  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.

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

    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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com

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

    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. Show all issues

    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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com

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

  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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com

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

    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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

akim.ruslanov
akim.ruslanov
david
  1. 
      
  2. Show all issues

    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. Show all issues

    This should probably be set in initialize instead of here.

  4. Show all issues

    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. Show all issues

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

  6. Show all issues

    Let's split this up:

    this.svgSize = {
        width: this.viewBox.width,
        height: this.viewBox.height,
    };
    
  7. 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');
        });
    
  8. Show all issues

    What's going on here?

  9. Show all issues

    Add a blank line between these.

  10. Show all issues

    Too much indentation in here.

  11. Show all issues

    Add a blank line between these.

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

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

    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. Show all issues

    Indent as such:

    this.viewBox = {
          x: ...,
          y: ...,
    };
    
  15. Show all issues

    Add a trailing comma here.

  16. Show all issues

    Add spaces around - operator.

  17. Show all issues

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

  18. Show all issues

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

  19. Show all issues

    Typo?

  20. Show all issues

    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

  21. Show all issues

    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

  22. Show all issues

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

  23. Show all issues

    This needs a trailing comma.

  24. Show all issues

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

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

  26. Show all issues

    Add a blank line between these two.

  27. Show all issues

    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.

  28. Show all issues

    Same here.

  29. Show all issues

    Add a blank line between these two.

  30. Show all issues

    And here.

  31. Show all issues

    And here.

  32. Show all issues

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

  33. Show all issues

    This new blank line should be removed.

  34. Show all issues

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

  35. Show all issues

    This new blank line should be removed.

  36. Show all issues

    This new blank line should be removed.

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

  37. 
      
akim.ruslanov
Review request changed
Change Summary:

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

Commits:
Summary ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars
32439633178192f77065896b91aae703ece28e99 aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars
32439633178192f77065896b91aae703ece28e99 aruslanov@fispan.com
missed out on some small whitespaces and semicolons
36ebf8bb1450f9afb7b34ba7e4eba62d934f463a aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars
32439633178192f77065896b91aae703ece28e99 aruslanov@fispan.com
missed out on some small whitespaces and semicolons
36ebf8bb1450f9afb7b34ba7e4eba62d934f463a aruslanov@fispan.com
comments are scaled with SVGs
695325381aae48ddf9dc8aa1ce7861c7a8333182 aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars
32439633178192f77065896b91aae703ece28e99 aruslanov@fispan.com
missed out on some small whitespaces and semicolons
36ebf8bb1450f9afb7b34ba7e4eba62d934f463a aruslanov@fispan.com
comments are scaled with SVGs
695325381aae48ddf9dc8aa1ce7861c7a8333182 aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars
32439633178192f77065896b91aae703ece28e99 aruslanov@fispan.com
missed out on some small whitespaces and semicolons
36ebf8bb1450f9afb7b34ba7e4eba62d934f463a aruslanov@fispan.com
comments are scaled with SVGs
695325381aae48ddf9dc8aa1ce7861c7a8333182 aruslanov@fispan.com
fixing whitespaces
a7bcd730e93043c7ce57d1a96ee746365eb05c60 aruslanov@fispan.com

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 ID Author
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars
32439633178192f77065896b91aae703ece28e99 aruslanov@fispan.com
missed out on some small whitespaces and semicolons
36ebf8bb1450f9afb7b34ba7e4eba62d934f463a aruslanov@fispan.com
comments are scaled with SVGs
695325381aae48ddf9dc8aa1ce7861c7a8333182 aruslanov@fispan.com
fixing whitespaces
a7bcd730e93043c7ce57d1a96ee746365eb05c60 aruslanov@fispan.com
jshint issues fixed
25b54945aa2ea411da42d7a84e48506aae248d5b aruslanov@fispan.com
wip initial
2570c36b4c1cd76fcefc9b869ac25065dfef62ba aruslanov@fispan.com
mousewheel works in parent view
48cb0d906cb2bec1390c1f6296c8bb161cfac556 aruslanov@fispan.com
clean up
753647f2c2d2688b96cd18dd421d114e37b4238f aruslanov@fispan.com
fixed event handling
e75fb177bb8e7094fbbb4bae97c8731335799bd7 aruslanov@fispan.com
onload question
1dfdaf59151afa2452efbd34176085d7082c408b aruslanov@fispan.com
fixed jshint problems
9be62ebc9f064d77463e3d958cdbb383dd5e32a3 aruslanov@fispan.com
dragging ability added; new button to switch between spanning and commenting added
1ca946ca853b4c18f6be2032dcc607a1ee6a71c9 aruslanov@fispan.com
fixed dragging issue; reviewing the bug with scaling and dragging
8d0966d5ca11e4b18e6306fe7c0a1b1ed569681c aruslanov@fispan.com
fixed jshint issues
686ee924c529be45913ccd98be573922e2fea000 aruslanov@fispan.com
figuring out fetching within render
b4d94dc327afb9e25218d290576546b8bc0014eb aruslanov@fispan.com
finished transferring to SVGElementsAPI; buttons flushed out
8766e47c39664558f544aaf7bce3f6c9b1e5025e aruslanov@fispan.com
fixing some jshint issues
8debba864495af432a5fcbe60790abb4ece4b277 aruslanov@fispan.com
added documentation; cleaned up code
be17d29350ea52b0ebdd64b9d8b01379456c5ec3 aruslanov@fispan.com
fixed whitespace; added a method to stringify viewBox
009fbd5cf143509802c7b134ebceec40af20c982 aruslanov@fispan.com
fixed more whitespaces
80be40ed63dbe7eabf8ae06e853b374a27583d90 aruslanov@fispan.com
small bug with new method
befa38482a9ed08523e51f500dfffd1d73712644 aruslanov@fispan.com
mend
cbaea543b76976a7077252f4085eba1550c7949e aruslanov@fispan.com
Changes according to David's comments; made sure everything wraps nicely with 80 chars
32439633178192f77065896b91aae703ece28e99 aruslanov@fispan.com
missed out on some small whitespaces and semicolons
36ebf8bb1450f9afb7b34ba7e4eba62d934f463a aruslanov@fispan.com
comments are scaled with SVGs
695325381aae48ddf9dc8aa1ce7861c7a8333182 aruslanov@fispan.com
fixing whitespaces
a7bcd730e93043c7ce57d1a96ee746365eb05c60 aruslanov@fispan.com
jshint issues fixed
25b54945aa2ea411da42d7a84e48506aae248d5b aruslanov@fispan.com
i hate whitespaces
94ccf4b1ce3c255929f9a8f2c491a396a60c8243 aruslanov@fispan.com

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

  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.

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

  5. Show all issues

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

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

  7. Show all issues

    Comments should always end in a period.

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

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

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

  11. Show all issues

    These can be const.

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

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

  14. Show all issues

    These can be const.

  15. Show all issues

    These can be const.

  16. Show all issues

    There should be a space before/after operators.

    This applies to other lines below.

  17. Show all issues

    These can be const.

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

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

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

    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. Show all issues

    Blank line between these.

  22. Show all issues

    Whitespace on either side of operators.

    Check the rest of the change for whitespace around operators.

  23. Show all issues

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

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

  25. Show all issues

    "the" viewBox of "the" SVG.

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

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

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

    This is indented too far.

  28. Show all issues

    This will need a trailing comma.

  29. Show all issues

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

  30. Show all issues

    This needs a trailing comma.

  31. Show all issues

    The parameters are no longer aligned.

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

    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)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

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

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

    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. Show all issues

    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. Show all issues

    "trigger a corresponding handler".

  38. Show all issues

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

    if (this.enableCommenting) {
        ...
    } else {
        ...
    }
    
  39. Show all issues

    "trigger a corresponding handler"

    Same with some other comments below.

  40. Show all issues

    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)
     
     
     
     
     
     
    Show all issues

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

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

  43. Show all issues

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

  44.