Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

[wip] Allow comment margin to appear on editable diff view #2239

Closed
wants to merge 3 commits into from

Conversation

jcansdale
Copy link
Collaborator

@jcansdale jcansdale commented Feb 25, 2019

What this PR does

Previously the comment margin would only appear on pull request files that are open in a "full" editor window. This pull request allows the comment margin to also appear on editable diff windows.

How to test

  1. Check Show PR comments in editor margin feature flag option
    image

  2. Open a pull request and checkout its branch
    image

  3. Right-click on a pull request file and View Changes in Solution
    image

  4. Toggle the check-box in the margin
    image

  5. Click comment glyph in margin to view comment
    image

Observations

This functionality wasn't ready for prime time and has remained behind a feature flag.

What didn't work well

  • Difficult to keep track of files/comments that are part of pull request
  • The comment might no longer be relevant to the code it appears on
  • Can't comment on code until it has been pushed

What worked well

  • Extra space in full editor window to view comments

Possible future work

  • Being able to mark comment threads as resolved
  • Map of comments on pull request files
  • Map of changed files in pull request

@jcansdale jcansdale changed the title [wip] Allow comment margin to appear on editable diff view Allow comment margin to appear on editable diff view Feb 25, 2019
@meaghanlewis meaghanlewis added this to the 2.9.0 milestone Feb 26, 2019
@jcansdale jcansdale changed the title Allow comment margin to appear on editable diff view [wip] Allow comment margin to appear on editable diff view Mar 5, 2019
@jcansdale
Copy link
Collaborator Author

Comments on editable files are no longer a thing (see #2408).

Closing.

@jcansdale jcansdale closed this Oct 25, 2019
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.

None yet

3 participants