I have a change up for review that adds registration. It also adds a basic ImageReviewUI. I recommend creating a branch, applying my change, and basing your work on that one.

Two blank lines.

This isn't valid.

Always limit lines to < 80 chars.

Space before =

Do you need "new" and "2" ?

Insert alphabetically.

Same.

I'm not sure why you changed this (and I believe it will break other UIs). Why did you namespace these instead of just using the orig_image directly?

You can also write this as:

if self.orig_image:

Must be 2 spaces

I am not sure if you need a new URL and function for image diffing. It seems like the "diff" function in reviews/views.py finds the review request, gets all the diffsets and then uses the most recent one. 

I am not sure if it can be done but you might be able to do that in the Review UI itself. That would let you cut back the new URL pattern you introduced as well as this function.

Must be 2 spaces

Small nitpick but I would use 6-digit codes instead of 3 (for the sake of clarity). Writing #FF7777 is much more understandable than #FF7. It will also become consistent with all your other color codes (which are all 6-digit)

It might be better to use variables for these magic numbers to describe what these values are. The number "12" and "30" are repeated often and using a variable will make it much easier for you to modify these values.

This breaks pep8 max line length of 79 chars; you can break it down to:

    orig_file_attachment = get_object_or_404(FileAttachment,
                                             pk=orig_file_attachment_id)
    new_file_attachment = get_object_or_404(FileAttachment,
                                            pk=new_file_attachment_id)

Should this subclass ImageReviewableModel (imageReviewableModel.js) instead?

Should this subclass ImageReviewableView?

This looks awesome. Any chance you can post screenshots of the different views when the image remains the same size?

Shouldn't we return the base classe's extra context no matter what? Something like:

extra_context = super(ImageReviewUI, self).get_extra_context(request)

if self.orig_image:
    extra_context['orig_image'] = self.orig_image

return extra_context

Michelle:

This looks *awesome*. I'm really excited to use it. :)

-Mike

For all of these rules in here - I think we have a pattern in here we we alphabetically order the properties in each rule.

Nit: indented too far.

You put documentation on the two functions above - you should do it for the rest of these as well.

The formatting here looks a bit off - I think this needs to be indented more.

Same as above.

Nice use of promises. :)

Nit - indentation

Nit - indentation - plus, I think it needs a newline afterwards.

Needs var

Setting .src on an Image is asynchronous. I don't think you can trust that .width and .height is available on your images until the images have at least started loading.

I know that, in this case, you'd be fetching images from the cache, but that's still asynchronous, and you're still going to have a tiny race.

You might want to use promises again here.

Same as above wrt the race.

Too much indentation

Just wanted to say...this is a really cool function.

Nit - spaces on either side of the +'s in here.

Indentation is still off here.

Why is the text-align an inline style?

This should be two lines. self.template_name should be on the same line as render_to_response.

No need for parens.

Can you do "XXX Temp URL ..."? This will show up styled in many editors, making it easier to see, since this change will probably start off in a pushed branch for now.

These should probably be top-level.

I thought we included a jquery-ui stylesheet. Do we not?

Should inherit from FileAttachmentReviewable.

If we're not doing anythign special here, you can remove this.

This can grow out of control. I'd suggest doing something like;

this._$image_diffs.children().hide();
this._$view_modes_list.children().removeClass('active');

One var statement only, with each variable on its own line. For example:

var image_1 = blah,
    image_2 = blah,
    image_width_1 = blah;

vars must be declared at the top of a function. (They can be assigned later).

In reality, JavaScript moves vars under the hood to the top anyway, so this is to reflect it and prevent certain types of problems.

Can you go through the rest of the code and make sure the vars behave the correct ways? One var per scope level, defined at the top of the scope.

Should be:

image_frame_2.css({
    'margin-left': blah,
    'margin-right': blah
});

Same with the ones below.

Align parameters.

Should do:

this._$wipe_view
    .width(...)
    .height(...);

Only reference the element once.

You won't need to append "px" when doing this. It'll handle that itself.

Should be two lines, with the first parameter on the first line.

Use dictionary form:

self.blah
    .css({
        opacity: percent,
        filter: ...
    });

Do:

this._$onion_view
    .width(...)
    .height(...);

No "px"

Two lines total.

Hi Michelle,

Just a question that came up while we were discussing this change offline. Is the logic for your image diffing based on any existing code out there? If so, do you know what the license is to that code?