[go: up one dir, main page]

Multi-line merge request comments: 2. Highlight lines spanned by each comment

Problem to solve

After the first iteration of adding basic UI via Dropdowns on MR Comments, it's not very clear what's the interval of lines spanned by each comment.

We should highlight the intervals upon hover and when focusing on/authoring one comment.

Implementation details / Rollout plan

  • Attention: need to test the themes properly.
  • Note: we can use an element with pointer-events: none; using rgba() as background color.
  • Note: while leaving the comment, they’ll be highlighted. Clicking/hovering on a comment, highlights too. Blurring the comment, will turn off the highlights.
  • Is blocked by: Refactoring of Diff lines using non-table markup (issue to be added)
    • Includes: Comment bubble will be shown/hidden via CSS

Further details

Commenting on multiple lines does increase the complexity of performing a code review because it changes the decision making process for each discussion.

  • Currently, a user need only decide which line is most relevant, click and add their comment.
  • Multiline commenting, instead, requires a user to decide if this should be a single/multiline comment, then decide where to start of the range, and then the end of the range, and finally add their comment.

The question is, what percent of the time is a multi line comment preferable, and for that benefit is the increased complexity worth it?

Historic discussions

Some questions posed by Douwe earlier, with answers:

  1. How do we select that? Click 3, drag and hold until 8? What happens if someone wants a bigger range that doesn't fit inside the viewport?
  2. Click and drag. We shouldn't encourage selecting entire files, so whatever we get when we implement this (be that, you can't do it, or the viewport slowly scrolls down, as happens with selections)
  3. How do we display what lines a specific discussion applies to?
  4. State explicitly and show the range up to X lines
  5. Can people comment on a range that spans a collapsed region?
  6. Yes, I think this should be possible (another issue relates to this)
  7. Can people comment on intersecting ranges? (3-8 and 6-12)? How would they specify their range? How do we display that?
  8. Maybe for the first iteration not? In general I think this is easy to represent in the db, just hard in the interface.
  9. Can people comment on a line inside a range that has a discussion? How do we allow that to be created? How do we display that?
  10. Ideally yes, same as previous question. Otherwise skip for first iteration. Depends on UX more than anything.
  11. Can people comment on the last line of a range that has a discussion? How do we allow that to be created? How do we display that?
  12. What happens if someone adds a line in the middle of the range. Does the discussion become outdated, or remain active?
  13. What happens if someone removes a line in the middle of the range. Does the discussion become outdated, or remain active?
  14. What happens if someone changes a line in the middle of the range. Does the discussion become outdated, or remain active?

Proposal

Selecting a range

  1. #211255 (closed)
  2. #218319 (closed)

Visualizing the range, and overlapping ranges (this issue)

  1. Highlight line range when writing a comment/reply
    1. While the comment text is focused, the range to which it applies is highlighted.
  2. Can be done in separate iteration: Highlight line range when hovering a published/pending comment
    1. Hovering over a discussion thread shows the range to which it applies. Hovering over a line of code does not trigger a highlight event as there may be multiple matching comments.
Hover a comment to see selection range Overlapping comment ranges
Artboard_20190607_120213-1 Artboard_20190607_120046

Marking discussions outdated

All discussions relating to a line that is changed are marked out of date.

We should measure the number of overlapping discussions with ~"telemetry 🔮" and consider revising this behavior in the future.

Availability & Testing

  • As in the MVC, no risk to availability is expected, but we should check how the feature performs on large diffs with many comments.
  • However, this change comes with considerable risk to user satisfaction should anything go wrong, given the popularity of this feature and the heavy use of comments in merge requests.
    • That risk can be mitigated through extensive new unit and integration coverage to cover known troublesome edge cases. Particularly interactions with expanding lines and changes to lines before, after, within, and spanning the commented range. Feature tests may be required given the nature of the UI actions involved.
    • Although selecting over collapsed regions is optional for this iteration, we'll need to confirm that attempting to do so doesn't cause problems.
  • No need for a new E2E test (and isn't recommended given the complexity of the UI interaction), and the change isn't expected to affect existing E2E tests.

Links / references

Interested customers (CARR/IACV):

Edited by Pedro Moreira da Silva