[go: up one dir, main page]

Highlight multiple lines to create multi-line merge request comments

Problem to solve

Commenting on a single line is great for simple kinds of code review feedback, but frequently a comment is actually addressing multiple lines in the diff, perhaps a portion of a logic block, a paragraph of prose or an entire function. This forces users to chose a single line to provide feedback, but the feedback may in fact be resolved with a change to a different line.

The drop down introduced in #211255 (closed) is a great MVC, but it still presents some cognitive overload to the user as they have to match the code they want to include with the line number the code is on.

User experience goal

Very easy to define an interval of a comment in an MR from the MR diffs view.

Proposal

When highlighting multiple lines in diff, offer one comment bubble to comment on the range

CleanShot_2020-10-27_at_16.43.55

old proposal

Clicking and dragging is an accepted and learned pattern.

Range selection is by performed by clicking the start of the range, and shift clicking the end of the range. This is consistent with typical text/range selection patterns.

  1. Click the line number of the first or last line of the range
  2. Shift-Click the line number of the last or first line of the range respectively or drag to final line number.

Edge cases:

  • selecting over collapsed regions: Optional. Support will likely eventually added, but the there is no requirement for this in the first iteration.

Further details

Final implementation might depend on further discussions with UX during development and review.

Documentation

Update documentation of MR Multi Line comments to address the final state of funcionality.

Availability & Testing

  • Click-and-drag actions tend to be flaky when automated, so if a feature spec is added it might be wise to run it several times in CI before merging to be confident it's stable.
  • If there's no feature spec a simple E2E test should be added (@mlapierre can do that).
  • Run package-and-qa to confirm no other commenting-related QA tests are affected.

What does success look like, and how can we measure that?

Easy to add a comment spanning multiple lines.

What is the type of buyer?

Is this a cross-stage feature?

Links / references

Edited by Daniel Gruesso