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

Pull Request conversation view #2017

Merged
merged 73 commits into from Feb 21, 2019

Conversation

@grokys
Copy link
Contributor

commented Oct 30, 2018

Work in progress for attn of @donokuda.

To show the conversation view, right-click on an item in the PR list and select "Open Conversation" (note: this is just a temporary method to open the view, see Open Questions below)

Progress so far

2018-11-01_19-14-34

2018-11-01_19-18-45

Open Questions

  • I've added a button to close/reopen the pull request, but closing/reopening doesn't reflect the changes yet in the GitHub pane, and doesn't add/remove the entry from the PR list
    • Syncing with other parts of the UI will take a bit more work
    • Given that we don't have "Merge" yet shall we punt this?
  • We need to decide what to do with the PR description in the GitHub pane, and how to navigate to the conversation view from there
    • Remove the description from the GitHub pane? Display a summary with a link to the conversation view (summary being e.g. just the first paragraph?)

grokys added some commits Oct 26, 2018

Show document pane for PR conversation.
It's currently empty, but it shows. Only supported method of showing is currently right-click on PR list.
Display PR details in document pane.
Next need to refactor out a common base class and create 2 different PR detail view models.
Load PR timeline from GraphQL.
And display its commits and comments in the conversation view.
@codecov

This comment has been minimized.

Copy link

commented Oct 31, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@9b9a90a). Click here to learn what that means.
The diff coverage is 14.08%.

@@           Coverage Diff            @@
##             master   #2017   +/-   ##
========================================
  Coverage          ?   38.7%           
========================================
  Files             ?     423           
  Lines             ?   17940           
  Branches          ?    2451           
========================================
  Hits              ?    6944           
  Misses            ?   10444           
  Partials          ?     552
Impacted Files Coverage Δ
...rc/GitHub.App/ViewModels/CommentThreadViewModel.cs 83.67% <ø> (ø)
...dels/GitHubPane/PullRequestUserReviewsViewModel.cs 67.16% <ø> (ø)
src/GitHub.Exports/Models/CommentModel.cs 100% <ø> (ø)
...itHub.InlineReviews/Services/PullRequestSession.cs 77.24% <ø> (ø)
src/GitHub.App/Models/RemoteRepositoryModel.cs 59.25% <ø> (ø)
.../GitHub.Exports/Models/PullRequestListItemModel.cs 87.5% <0%> (ø)
src/GitHub.App/Services/FromGraphQlExtensions.cs 0% <0%> (ø)
src/GitHub.Exports/Commands/OpenIssueishParams.cs 0% <0%> (ø)
...p/ViewModels/Documents/PullRequestPageViewModel.cs 0% <0%> (ø)
src/GitHub.App/Models/PullRequestModel.cs 33.33% <0%> (ø)
... and 25 more
@jcansdale

This comment has been minimized.

Copy link
Collaborator

commented Nov 2, 2018

I'm excited so see this going in! 😃

Workflow wise, now I have the option to view using Open Conversation, the next most likely action I'll want to preform is Edit in Browser.

image

I'd almost be inclined to replace the Open in Browser option with an Edit in Browser option (that starts editing the MD - if such a thing is possible). Is there a URL for that?

For some reason I keep heading to the Open in Browser link at the bottom, rather than the Open Conversation link. @donokuda are users more likely to click the top/bottom commands on a menu? Or is it just me? 😉

Having links to individual commits is great! Could we make the text clickable as well, like on dotcom?

image

Once nice thing is, once you've drilled down to a file diff, you have to option to Open File in Solution and navigate to the change of interest (assuming it still exists).

image

I wonder how difficult would be to leverage a read-only Visual Studio editor component for syntax highlighting? Is markdig easily customization like this?

image

Maybe @jaredpar would give us a sanity check/tips on how to create a read-only editor component like this? 🙏

It would be nice if we could get links wired up to the Open from GitHub command, so the user can navigate to lines/ranges in the local solution!

image

I think this kind of integration is where we can add value when viewing PRs/issues inside Visual Studio.

@jcansdale

This comment has been minimized.

Copy link
Collaborator

commented Nov 2, 2018

Something I really miss is emoji support. Could we have a button or even just a reminder to click Windows Key + . to bring up the Windows 10 emoji picker? 😜

image

@jaredpar

This comment has been minimized.

Copy link

commented Nov 3, 2018

Maybe @jaredpar would give us a sanity check/tips on how to create a read-only editor component like this

Not quite sure what you were looking for advice on here. Could you elaborate?

@jcansdale

This comment has been minimized.

Copy link
Collaborator

commented Nov 5, 2018

@jaredpar,

Not quite sure what you were looking for advice on here. Could you elaborate?

I remembered you wrote the https://github.com/jaredpar/EditorUtils library for hosting the Visual Studio editor outside of VS. What I'm wondering about here is a potentially simpler scenario (hosting it inside VS).

At the moment we're using Markdig for rendering .md content inside Visual Studio. Unfortunately it doesn't have built in syntax highlighting.

I was wondering how feasible it would be to create a read-only WPF editor control with syntax highlighting, that corresponds to a markdown code block? We could then render any code blocks in pull request/issue in a similar way to Visual Studio. The highlighting and color themes would be the same as Visual Studio so it wouldn't look out of place.

Is this realistic or do you think there are performance/complexity issues that are likely to bite us?

@jaredpar

This comment has been minimized.

Copy link

commented Nov 5, 2018

What I'm wondering about here is a potentially simpler scenario (hosting it inside VS).

Yes that is feasible. Within VS the editor is just there. You can grab all of the factory interfaces from the MEF container and just create the IWpfTextView instances directly.

grokys added some commits Nov 8, 2018

Implement closing/reopening a PR.
This state is not synced with the PR details pane or the PR list currently though.
@grokys

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

@meaghanlewis yes, this is definitely a feature we should have! I did look into implementing it a while ago, but never finished it. It would involve writing a markdig extension, which shouldn't be too difficult; would just require the time to be allocated to it.

@donokuda

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

One thing I noticed while testing. I'm not sure what the correct functionality should be here, Don says he expected to open the browser, I expected it to open the Detail page.

Floating this idea:

giphy

If I click on a link, I'd mostly expect the page that the action took place to update instead of something on the side. However, I probably expect that since the page itself looks and feels like closer to a webpage.

We could consider doing both: navigate the conversation view to the corresponding pull request and also navigate the GitHub panel to show the details view with relevant information to the PR.

There are a few risks that I anticipate (which we would want to consider validating with some folks): 😬

  1. When I'm focusing on a specific pull request, it's confusing to have the panel navigate without me interacting with it. If I'm not aware that the panel has updated, I might be disoriented or accidentally read irrelevant information.
  2. It's not clear how to get back to the previous screen in the pane. (The solution here would be to use the navigation bar above, so this would mostly be testing if that functionality is discoverable.)
  3. People don't expect the conversation page to change. 👈I doubt this is a problem, but I also understand that my hunch could be wrong.

I am leaning towards navigating both the conversation page and the GitHub panel to the pull request, but I'm open to feedback in case I'm missing something. 😄

@jcansdale

This comment has been minimized.

Copy link
Collaborator

commented Feb 12, 2019

@donokuda I've been thinking about how I use the current View on GitHub link. I pretty much always use it after creating a PR to edit the markdown description and to add images. This is something the current conversation view doesn't allow and it would take a lot effort to get feature parity with dotcom. I think this is a pretty important use-case/workflow.

How about rather than replacing or combining the View on GitHub / View Conversation functionality, we keep the View on GitHub link and take inspiration from the comments icon on the dotcom PR list.

image

I link showing the number of comments gives people an incentive and invites them to click. It would also be clear that clicking on the comment link would show those comments.

I think we've always been pretty explicit about when a command will open an external browser (for both menus and links). I'd be hesitant to introduce a command that both opens an external browser and does an action inside Visual Studio.

image

In fact we should have a common command/keyboard shortcut to Open on GitHub no matter the context (if we don't already). 🤔

@donokuda

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@jcansdale 👍 Sure, let's try that idea out! 😄

My only concern is if the text isn't blue (like the links around it), then folks might assume they cannot interact with it (and that it's only informational.)

Would you be open to making it a blue link?

@jcansdale

This comment has been minimized.

Copy link
Collaborator

commented Feb 12, 2019

@donokuda,

Would you be open to making it a blue link?

Absolutely! The main thing was the comment icon/number of comments. I copy/pasted from dotcom. 😉

@meaghanlewis meaghanlewis added this to the 2.8.0 milestone Feb 12, 2019

@jcansdale

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

@StanleyGoldman,

One thing I noticed while testing. I'm not sure what the correct functionality should be here, Don says he expected to open the browser, I expected it to open the Detail page.

Is this the link you're referring to above?

image

I'd expect the # link on the conversation view to open in a browser, just like the link in the the notification does on the left. In fact, could we include the owner/repo in the conversation view link as well to add a little more context?

donokuda and others added some commits Feb 13, 2019

Don't collapse Reviewers or Checks sections
The Reviewers section contains an `Add your review` button which I
don't think should be hidden by default.
The Checks section is the only place where build status is surfaced and
contains a link to Details for each build.
Set the pull request WebUrl and OpenOnGitHub
Make hash-link at top pull request page open on GitHub.
Merge pull request #2229 from github/donokuda/moving-some-cheese
Add pull request comment count link and add back in description section

Fixed

@jcansdale
Copy link
Collaborator

left a comment

Tested on 2015, 2017 and 2019. 🎉

Let's 🚀 it!

@StanleyGoldman StanleyGoldman merged commit 4464697 into master Feb 21, 2019

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
github.VisualStudio Build #20190221.4 succeeded
Details

@StanleyGoldman StanleyGoldman deleted the feature/pr-conversation branch Feb 21, 2019

@StanleyGoldman StanleyGoldman changed the title PR conversation view Pull Request conversation view Feb 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.