Galooshi / happo

Visual diffing in CI for user interfaces
505 stars 16 forks source link

Improve UI for reviewing diffs #61

Closed trotzig closed 8 years ago

trotzig commented 9 years ago

Right now, we just inject the full diff image produced by diffux-core onto the page. It is sometimes hard to understand what the image is showing, especially for wide diffs. In the old Diffux project, we had a tool that allowed you to quickly switch between the before, after and diff image by simply hitting the x key. You could also click the tabs to switch. I think we should have something like that in the Diffux-CI project as well.

There are a few complexities here to consider:

Any ideas here are welcome!

lencioni commented 9 years ago

I kind of want to use React

I'd also like to use React.

Here's an idea: what if we built a separate npm package that we get hosted on cdnjs.com or some other shared place. Then the JS we are adding to the page and uploading to S3 will be minimal for each set of diffs.

As far as using Babel, there is a Babel gem that we could use to transpile our code pretty easily. We are using this for the style guide at Brigade and it was easy to set up and seems to be working well.

trotzig commented 9 years ago

Sure. I hesitate a little bit about breaking out a separate project for this purpose (diffux is already scattered), but it makes sense. We can make it a general image-diff-review module that you can use for other purposes as well. But I guess the diff image format used by Diffux is very specific, though it is mostly optimized to support larger, full-page, diffs. Maybe we should just abandon that format and instead create diff images on the fly when you review? I remember seeing a technique using css to accomplish the overlay diff (can't find it now) and with such we could just save the baseline and candidate images. That should give a good performance boost too.

<VisualDiffReview 
  before="path/to/baseline.png" 
  after="path/to/candidate.png"
/>

If we don't use the diff image from diffux-core, we could cut that dependency and just make a quicker pixel-by-pixel diff instead of the LCS thing. Trading precision for performance seems worth it here, doesn't it?

lencioni commented 9 years ago

Sure. I hesitate a little bit about breaking out a separate project for this purpose (diffux is already scattered), but it makes sense.

We could keep it in this project and just put it in a directory or something if we prefer. I don't think it matters much.

I remember seeing a technique using css to accomplish the overlay diff

Something like this maybe? http://franklinta.com/2014/11/30/image-diffing-using-css/

There's also stuff like Resemble.js if we decide to go down this route: http://huddle.github.io/Resemble.js/

Trading precision for performance seems worth it here, doesn't it?

Maybe, but on most runs we don't have any diffs, so I don't think we would actually see much real world benefit from this tradeoff. I think we should prioritize making it easy for people to understand the diffs--whatever that means. It might mean having an array of options to choose from to see the differences: a slider, flipping between them quickly, maybe an overlay, maybe something from Resemble.js?

trotzig commented 9 years ago

Something like this maybe? http://franklinta.com/2014/11/30/image-diffing-using-css/

Yes! Though now that I look at it, it seems a little limited.

Maybe, but on most runs we don't have any diffs, so I don't think we would actually see much real world benefit from this tradeoff. I think we should prioritize making it easy for people to understand the diffs--whatever that means. It might mean having an array of options to choose from to see the differences: a slider, flipping between them quickly, maybe an overlay, maybe something from Resemble.js?

Yeah, maybe the visual aid for diff reviewing is worth it. We're mosly using smaller components in the Brigade codebase but I can see others using full-page screenshots more. Let's use the diff image that we have today and see how far we can go. And having the module live inside the diffux-ci repo sounds good to start with.

trotzig commented 8 years ago

I see this as top priority for this project right now. I've heard a few times that the diff images are hard to interpret, and it would be good to make that UX better.

lencioni commented 8 years ago

I see this as top priority for this project right now. I've heard a few times that the diff images are hard to interpret, and it would be good to make that UX better.

Yes, I totally agree. I think it would be good to have different ways to view the differences. Some that come to mind are based on what GitHub does:

trotzig commented 8 years ago

I just came across an example of a diff where it is really hard to see what changed:

difficult_diffux_diff

Being able to quickly switch between before and after (without the gutter) would probably help here.

trotzig commented 8 years ago

Work on this needs to happen soon! I've got a few ideas of how we can move forward without making major investments, yet still make the diff UI a lot better:

The gutter and gaps were useful when happo (diffux) was used to screenshot full pages. That's not its main use-case anymore. Right now they mostly make it harder to see differences. They're still useful for the overlay however.

By keeping everything contained in one image file (as we do today), we avoid uploading redundant info to s3. We also make it easy to share diff images. If we can make the assumption that the full diff image consists of three parts of equal widths, the js code should be simple to write.

I'd prefer if we explored the plain js option first, before diving into a fully fledged React+babel project.

lencioni commented 8 years ago

I think we should strive for a diff viewer that allows you to pick from side-by-side, swipe, crossfade, and diff image. I think we could do this with the single image format you suggest but I think it would be better to split it out into three images: previous, current, and diff image (currently the middle panel). That still avoids uploading redundant info to S3.

As for ease of sharing diff images, I think it might be better to just share a link that goes directly there (enabled by f835123e057497056bfd5c82b9dccd1cf435b819). If we want to go a step farther and make shareable composite images for downloading and sharing, we might be able to do this in JS on the fly. But, I'm not really sure how useful that will be.

I found this project that might be a good start, but I think it might be just as easy to write this stuff ourselves. https://github.com/cezary/react-image-diff

I'd prefer if we explored the plain js option first, before diving into a fully fledged React+babel project.

I'm on the fence about this. I think it would be really nice to use React for this. We can do this without Babel if we want (either avoid JSX or use the browser transform). If we put it all in one file and inline it into the document, it shouldn't be too bad to manage. We can pull React on to the page from cdnjs or npmcdn so we don't have to upload it to S3.

trotzig commented 8 years ago

We now have the following views:

I think we should add a simpler diff view that is handled in the browser instead of using the diff from the server.

Apart from that, I think it's worth adding keyboard nav (jump between diffs, switch views) as well.

lencioni commented 8 years ago

I'd like to add a crossfade view, and maybe a toggle view (like crossfade, but it goes from one image to the next instantly). The toggle view would work well with a keyboard shortcut to toggle.

I'm also +1 on generating the diff in the browser, but I think we should preserve the LCS approach. We can do multiple diff views as well, but I think it is valuable to have a diff view that uses LCS.

trotzig commented 8 years ago

Yeah, I wasn't advocating for removing the LCS diff.

lencioni commented 8 years ago

I've split out some separate issues for some of the remaining enhancements we've discussed here.

138 #139 #140