Improve UI for reviewing diffs #61
Comments
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. |
|
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? |
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.
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/
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? |
Yes! Though now that I look at it, it seems a little limited.
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. |
|
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:
|
|
I just came across an example of a diff where it is really hard to see what changed:
Being able to quickly switch between before and after (without the gutter) would probably help here. |
We only use this in Diffux, not Diffux CI, so now that we've moved the code over from diffux-core, we can simplify a little. We may want to bring this back or something like it when we improve the UI for reviewing diffs (#61), but for now I want to keep things simple.
|
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. |
|
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 f835123). 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'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. |
We're about to make the diff review experience better. To make future enhancements easier, we're moving over the UI to React. This will allow us to work inside an environment that we're all (Galooshi members) familiar with. I've been pushing back on introducing unnecessary complexity into this project. We've had a fruitful discussion in #61 about possible ways to move forward, and what I'm presenting here is basically a middle ground between what @lencioni is suggesting and what I've been advocating for. I'm making the page depend on three externally hosted scripts: - React - so that we can write nice UIs - ReactDOM - so that we can hook up those UIs to the DOM - babel - so that we can use jsx, string templates, arrow functions and such. The in-browser transpilation done by babel isn't the fastest, but I don't see this as a bottleneck at this point. If the application continues to grow, it might be worth introducing a build step to the process. But at this point I don't think it's necessary. When I was porting over the image_slug method, I had some issue reimplementing it in javascript. So I decided to delegate to window.btoa() instead, which will simply base64 encode the string. This fixes a potential naming collision issue, but at the cost of making the URL less readable. Simplicity and robustness won this time, but there's a chance we'll move over to more readable slugs at some later point in time. This change shouldn't result in any differences in the UI (except for the already-mentioned slug change). Future commits will focus on improving it.
We're about to make the diff review experience better. To make future enhancements easier, we're moving over the UI to React. This will allow us to work inside an environment that we're all (Galooshi members) familiar with. I've been pushing back on introducing unnecessary complexity into this project. We've had a fruitful discussion in #61 about possible ways to move forward, and what I'm presenting here is basically a middle ground between what @lencioni is suggesting and what I've been advocating for. I'm making the page depend on three externally hosted scripts: - React - so that we can write nice UIs - ReactDOM - so that we can hook up those UIs to the DOM - babel - so that we can use jsx, string templates, arrow functions and such. The in-browser transpilation done by babel isn't the fastest, but I don't see this as a bottleneck at this point. If the application continues to grow, it might be worth introducing a build step to the process. But at this point I don't think it's necessary. When I was porting over the image_slug method, I had some issue reimplementing it in javascript. So I decided to delegate to window.btoa() instead, which will simply base64 encode the string. This fixes a potential naming collision issue, but at the cost of making the URL less readable. Simplicity and robustness won this time, but there's a chance we'll move over to more readable slugs at some later point in time. This change shouldn't result in any differences in the UI (except for the already-mentioned slug change). Future commits will focus on improving it.
We're about to make the diff review experience better. To make future enhancements easier, we're moving over the UI to React. This will allow us to work inside an environment that we're all (Galooshi members) familiar with. I've been pushing back on introducing unnecessary complexity into this project. We've had a fruitful discussion in #61 about possible ways to move forward, and what I'm presenting here is basically a middle ground between what @lencioni is suggesting and what I've been advocating for. I'm making the page depend on three externally hosted scripts: - React - so that we can write nice UIs - ReactDOM - so that we can hook up those UIs to the DOM - babel - so that we can use jsx, string templates, arrow functions and such. The in-browser transpilation done by babel isn't the fastest, but I don't see this as a bottleneck at this point. If the application continues to grow, it might be worth introducing a build step to the process. But at this point I don't think it's necessary. When I was porting over the image_slug method, I had some issue reimplementing it in javascript. So I decided to delegate to window.btoa() instead, which will simply base64 encode the string. This fixes a potential naming collision issue, but at the cost of making the URL less readable. Simplicity and robustness won this time, but there's a chance we'll move over to more readable slugs at some later point in time. This change shouldn't result in any differences in the UI (except for the already-mentioned slug change). Future commits will focus on improving it.
|
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. |
|
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. |
|
Yeah, I wasn't advocating for removing the LCS diff. |
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
xkey. 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!
The text was updated successfully, but these errors were encountered: