Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show commment on last line of range and update internals #146916

Merged
merged 2 commits into from
Apr 7, 2022

Conversation

alexr00
Copy link
Member

@alexr00 alexr00 commented Apr 6, 2022

Part of #146510

@alexr00 alexr00 self-assigned this Apr 6, 2022
@alexr00
Copy link
Member Author

alexr00 commented Apr 6, 2022

@rebornix feel free to review or not, as you wish :) I intend to merge this tomorrow (unless you have feedback to the contrary). I mostly wanted to give you a heads up on this change, which basically does two things:

  1. Shows the comment on the last line of the range. This is better than showing on the first line of the range because it doesn't split range with the comment editor zone. This is also what GitHub does.
  2. Wires up the internals of comments to support ranges instead of single lines, and exposes this through the "Add Comment on Current Selection" (formerly "Add Comment on Current Line") command. There's no new UX here, the command just now works on a selection instead of a line in order to take advantage of the multiline support.

Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only one suggestion for how we can avoid calculating the accurate comment range after editor model is modified.

if (newPosition.lineNumber !== originalRange.endLineNumber) {
// The widget could have moved as a result of editor changes.
// We need to try to calculate the new, more correct, range for the comment.
const distance = newPosition.lineNumber - this._commentThread.range.endLineNumber;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used the zone widget position since previously we only track single line and it serves good enough as markers. To accurately track the movement of the comment range, we may want to add a decoration for commentThread.range in the editor and reveive the latest range of the decoration on action execution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. I will leave as is for now though, since the decoration for a range requires some UX decisions that we have yet to make.

@alexr00 alexr00 merged commit d2663de into main Apr 7, 2022
@alexr00 alexr00 deleted the alexr00/multilineCommentInternals branch April 7, 2022 09:09
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants