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

It would be much more useful if the gutter emphasised the lines this PR affects, not the commentable range #392

Closed
Tyriar opened this issue Sep 10, 2018 · 8 comments · Fixed by #4409
Assignees
Labels
feature-request Request for new features or functionality on-testplan upstream/vscode ux
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Sep 10, 2018

I will often browse/tweak PR branches in the regular file view, I would find it so much more useful if the grey bar extended to only the lines that were changed. See this example:

screen shot 2018-09-10 at 11 46 04 am

I'd like it to show me these 7 lines instead:

screen shot 2018-09-10 at 11 47 38 am

I propose we change the gutter so there are 4 states instead of 3:

  • Uncommentable
  • Commentable + not marked (only shows when the gutter is moused over, or not as bold)
  • Commentable + marked
  • Comment exists
@Tyriar
Copy link
Member Author

Tyriar commented Sep 10, 2018

Mockups:

image

image

@Tyriar Tyriar added the ux label Sep 11, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Sep 11, 2018

cc @misolori

@Tyriar
Copy link
Member Author

Tyriar commented Feb 3, 2021

We don't have the label in this repo, but this is definitely a papercut 🩸 for me and one of my most wanted things.

@miguelsolorio
Copy link
Contributor

This was one idea I had that could improve this view a bit. The comments area could be moved to the left to have more space and decorators could be updated to show buttons. And hovering over a line could show the "add comment" action. That way there isn't ever any collision between the general glyph areas and commenting items.

Example
Kapture 2021-02-03 at 17 29 13

@Tyriar
Copy link
Member Author

Tyriar commented Feb 4, 2021

@misolori I'm more interested in the regular file view here which is useful not only when reviewing to see exactly which lines changed but also when working on my own PRs because it will show every line I've touched across all commits. Right now I can trim 3 lines off the top and the bottom because I know they are definitely not part of the changes, but that doesn't work at the start/end of files, or when changes happened within 3 lines of each other.

I'm of the opinion that we don't need any decoration to indicate the line is commentable unless it's being hovered, just like GitHub:

recording (11)

Right now it doesn't differentiate so I can't tell which lines changed from this view. Some set of lines in the red rectangle changed, but I don't know which:

image

We're instead prioritizing information that doesn't matter much (that you can comment on lines that didn't change) over information that does (which lines changed). For GitHub users I think it's understandable that you can comment on some lines outside the diff so people would discover this. But I don't mind which mockup we go with (#392 (comment)), as long as it's easy to see which lines a PR changed from the regular file view.

As for moving the line to the left, one benefit now is it's right next to the git diff indicators and it's showing very similar information, especially if this issue gets resolved:

image

@alexr00
Copy link
Member

alexr00 commented Oct 20, 2022

There must be something we can do from the extension to help with this. Will continue to investigate in November.

@alexr00 alexr00 added this to the November 2022 milestone Oct 20, 2022
@alexr00 alexr00 self-assigned this Oct 20, 2022
alexr00 added a commit that referenced this issue Nov 8, 2022
For #392
@alexr00
Copy link
Member

alexr00 commented Nov 11, 2022

I've tried out the options that the editor decoration API gives us, and the result isn't good enough since we can't put decorations in the gutter.

@joaomoreno suggested the dirty diff decorator, but since requires a whole source control it isn't a good option as is. We'll add this to the December plan and see if we can expose the dirty diff decorator outside of the source control, since it is the ideal UI for showing a diff indicator.

@alexr00 alexr00 modified the milestones: November 2022, December 2022 Nov 11, 2022
@joaomoreno
Copy link
Member

@joaomoreno suggested the dirty diff decorator, but since requires a whole source control it isn't a good option as is. We'll add this to the December plan and see if we can expose the dirty diff decorator outside of the source control, since it is the idea UI for showing a diff indicator.

There are currently two blockers for this:

  • It currently requires an SCM provider. This is easy to do.
  • It currently has no good solution for when multiple dirty diff decorator providers provide content. It currently just picks the first one it sees. This is harder, requires some UX thought.

alexr00 added a commit that referenced this issue Jan 18, 2023
alexr00 added a commit that referenced this issue Jan 18, 2023
alexr00 added a commit that referenced this issue Jan 19, 2023
* Register quick diff provider
Fixes #392

* Add quick diff provider
Fixes #392
@alexr00 alexr00 closed this as completed Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality on-testplan upstream/vscode ux
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@joaomoreno @Tyriar @RMacfarlane @miguelsolorio @alexr00 and others