-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Multiline comments #146510
Comments
@rebornix FYI since I think you also want this in notebooks and @laurentlb since I know you were interested in this. |
@alexr00 if I understand correctly, the api already supports this since the comment is based on range other than line. The UX in the editor and panel doesn't take the range into account though, only the start line of the range. |
@rebornix yes, the API does currently support this! I was just outlining all the parts. |
* Support multiline comments Part of microsoft/vscode#146510 * Fix test
Looks great! Is it possible to add explicit lines range to the header of comment as it is in GH with "Commenting on lines 63 to 66" as well? |
I do want to add this information somewhere, but the content of the header/title of the comment thread is owned by the extension that provides the comment. I will try adding the range info somewhere else in the comments widget. |
This should open the door to supporting I'm probably subscribed to an issue for that feature, however https://github.com/notifications/subscriptions is not showing that I am subscribed to any issues in this repo, which is clearly incorrect. I am subscribed to this issue for example. I wonder if somebody from the @microsoft org can poke GitHub about this. |
@alexr00 is it possible to extend comment block at the top for multiline comments as GH does? So it will be displayed above thread label (controlled by extension). |
@brianjmurrell this does indeed open the door for the suggestion feature in GitHub: microsoft/vscode-pull-request-github#603 |
@alexr00 Ha! That's the one, and yes, I am subscribed to it already. Too bad GitHub's tool to find all of your subscriptions is not listing anything from this repo. Thanks for the pointer! |
Hrm. This does not seem to be working for me. I try to click and drag but I just get a plus-sign that moves with my mouse cursor, no multi-line selecting for comment. |
Clicking and dragging on the + is not currently supported. Instead you make a text selection then click the +: #148214 |
Ahhh. I see. Thanks @alexr00! |
hey @alexr00 thanks for implementing this, works really well! just noticed one UX issue: when the comment is collapsed, it still shows the background and border on the code... fairly unpleasant to look at 😬 ideally, that styling should only display if the comment is expanded, right? :) |
I intentionally left the background and border when the comment is collapsed so that there was a strong indication that the comment is there and it's how other code review tools I've used in the past have worked. However, this is your code editor too and not just a code review tool. If there's lots of feedback that this is undesirable then we can change it. I could also have a separate theme color for when the comment is collapsed so that you could essentially hide the background and border in this case. |
I suspect it can be annoying to have the region always highlighted, in particular when the comment is resolved. I don't have data or user feedback yet, I can share it in the future. |
I find the highlight distracting even with the comment card open. I would prefer a setting for the highlight to only show up when the comment card is moused over or focused. |
Just FYI, #152067 will hide the range highlight when the thread is collapsed. |
The comments widget currently only allows commenting on a single line. For some use-cases of comments, such as code review comments, commenting on a range is not only beneficial, it's already supported by the underlying tooling (see GitHub pull requests for example). We should support a way to comment on a range. This support will require:
The text was updated successfully, but these errors were encountered: