Skip to content

Commenting on image diffs

Felipe Artur requested to merge issue_35873 into master

What does this MR do?

This MR primarily adds the ability to comment on image diffs. This includes merge requests and commit diffs. When you click on an image in the image diff, you can create discussions. Each time you click on the image (on a different area/coordinate), you will create a new discussion.

In addition, we also did the following on the frontend:

  • Consolidated the number of places where we initialize gl.ImageFile()
  • Converted btn-comment into a mixin, so that it can be reused for image diffs
  • Fixed an edge case on gl.ImageFile whereby occasionally the images wouldn't trigger the 2-up view because the image was loaded before gl.ImageFile was invoked

I don't see the collapse icon SVG anymore. How do I get my local system setup?

  1. Clone https://gitlab.com/gitlab-org/gitlab-svgs locally
  2. Checkout branch add-collapse-icon (at least until https://gitlab.com/gitlab-org/gitlab-svgs/merge_requests/5 gets merged)
  3. Run yarn install (first time only)
  4. Run yarn run svg
  5. Replace dist/icons.svg (gitlab-svg repo) with app/assets/images/icon.svg (this repo)

Are there points in the code the reviewer needs to double check?

Make sure everything looks 💯

Why was this MR needed?

Deliverable

Screenshots (if relevant)

Architecture

Diffs MR Discussion tab
Screen_Shot_2017-10-02_at_4.08.09_PM Screen_Shot_2017-10-02_at_4.08.13_PM
commenting on gif in MR diff tab commenting on gif in discussion tab
2017-10-02_16.14.22 2017-10-02_16.16.38
Replaced image in MR diff tab Replaced image in discussion tab
2017-10-02_16.18.29 2017-10-02_16.19.01
commenting on commit diff commit diff discussion in MR
Screen_Shot_2017-10-02_at_4.22.26_PM Screen_Shot_2017-10-02_at_4.23.21_PM

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

closes #35873 (closed), #38559 (closed)


Remaining Todos:

Remaining items:

Tests

Karma Tests

  • badge_helper
  • comment_indicator_helper
  • dom_helper
  • utils_helper
  • image_badge
  • image_diff
  • init_discussion_tab
  • replaced_image_diff
  • view_types
  • image_utility

Rspec Tests

  • Clicking on image creates image diff discussion
  • Resolved diff discussions load collapsed
  • Adding a comment to an existing diff discussion shows avatar badge
  • Polling a comment shows avatar badge
  • Image diff view modes show discussion badges (on discussion tab)
  • Image diff view modes show discussion badges (on diffs tab)
  • Image diff displays on discussion tab
  • Image diff displays on diffs tab
  • Image diff displays on commit diff

Pushed to next iteration

  1. avatar tooltips when hovering over badge on the image diff
  2. miniature version of the chat, when collapsing a discussion
  3. clicking on image badges should scroll to first note in the discussion (both discussion and changes tab, in a sensible way which tries to keep the image in context with the first comment in the discussion)
  4. add shadows to badge and cursor so that it's easier to visualize the depth
  5. create image diff discussions on the older image to another iteration
  6. clicking on the edge of the image in swipe view mode causes the comment indicator to get clipped (https://tppr.me/oAbGP)
  7. 2-up view isn't always placed side by side. We should change it so that it is. I think it is displayed on top of each other when the image width is over a certain size

Known UX debt (acceptable to move forward know that these exist, will create individual issues for each of them before this gets merged and approved by @dimitrieh)

  1. when you delete the first note of a discussion, the avatar badge does not get populated for the new first note (until you refresh the page)

  2. when you edit the first note of a discussion, the avatar badge does not get displayed after the editing is saved (until you refresh the page)

  3. If you do scenario 1 and then you switch to the other tab (discussion if you were on diff tab, diff tab if you were on discussion) and assuming those two tabs are already loaded, the avatar badge will not get populated (until you refresh the page)

  4. If you do scenario 2 and then you switch to the other tab (discussion if you were on diff tab, diff tab if you were on discussion) and assuming those two tabs are already loaded, the avatar badge will not get populated (until you refresh the page)

  5. Toggling nav bar and sidebar does not update the image badge positions to match their positions even though the image size changes

Edited by username-removed-408677

Merge request reports

Loading