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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR list polish: Move comment count to the right #1768

Merged
merged 21 commits into from Jul 13, 2018

Conversation

Projects
None yet
6 participants
@donokuda
Copy link
Member

donokuda commented Jun 29, 2018

(Targets @grokys's refactor/pr-list branch (#1737) but wanted to open this separately for discussion)

The goal of this change is to make the information a little more scannable by having the comment counts of each pull request be vertically aligned. A way to test my attempt is to scan down the list of pull requests, be able to identify which ones have comments, and know which ones have a high level of activity/discussion.

Before:

screen shot 2018-06-29 at 10 09 59 am

After:

screen shot 2018-06-29 at 10 08 48 am

/cc @simurai for 馃憖 and 馃挱(if you have any! 馃槃)

Icon="git_pull_request"
ToolTip="This is the current branch"
VerticalAlignment="Stretch"
Visibility="{Binding IsCurrent, Converter={ghfvs:BooleanToVisibilityConverter}}"/>

This comment has been minimized.

@donokuda

donokuda Jun 29, 2018

Author Member

I removed this to keep the number of icons down in the list, but I still think it's useful to call out if you have checked out a branch with a pull request associated with it.

One idea that @grokys and I kicked around is bolding the title. This is similar to a treatment used in the "Connect" page showing which repository that you are currently in:

screen shot 2018-06-29 at 10 44 30 am

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jul 2, 2018

One concern might be that when the comment counts are on the right, it's a bit harder to see where they belong to? To help that, there could be subtle borders:

with lines

Not super necessary, but something to consider.

@sguthals

This comment has been minimized.

Copy link
Contributor

sguthals commented Jul 2, 2018

I like the subtle border @simurai

@donokuda

This comment has been minimized.

Copy link
Member Author

donokuda commented Jul 3, 2018

Pushed up a couple of changes to try out adding a border and vertically centering the comment count. I think it solves a handful of grouping issues that I was experiencing earlier.

That said, I wonder if having the list item separator feels out of place... Part of that is mostly because I haven't really seen examples of this type of treatment around Visual Studio.

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jul 4, 2018

I wonder if having the list item separator feels out of place...

Hmm.. Is it that in VS borders are more used to separate entire sections and not just "list items"?

Another alternative might be to not have a "column" on the right, but move the comments count next to the PR number. Then you can still scan them by going down a more or less straight line.

screen shot 2018-07-04 at 12 00 21 pm

Also added a bit more space to not have the comment count and month clash with each other.

@jcansdale

This comment has been minimized.

Copy link
Collaborator

jcansdale commented Jul 4, 2018

Something I'm definitely missing from the new version is clickable hyperlinks to PRs on GitHub:

image

Is this something that could be re-enabled?

@jcansdale

This comment has been minimized.

Copy link
Collaborator

jcansdale commented Jul 4, 2018

Now this is looking closer to .com, something that is painfully missing is the CI status:

image

If all this did was open the same link as it does on .com, having CI status would be so useful! I know this info is available via GraphQL. 馃槃

@grokys

This comment has been minimized.

Copy link
Contributor

grokys commented Jul 4, 2018

Something I'm definitely missing from the new version is clickable hyperlinks to PRs on GitHub

As mentioned on the PR list PR: #1737 (comment)

This was by design. The list is now a selectable list, and selectable lists don't generally have inner clickable items. I was thinking of adding a context menu with "Open on GitHub"?

That context menu has been added.

@jcansdale

This comment has been minimized.

Copy link
Collaborator

jcansdale commented Jul 4, 2018

@grokys sorry I missed your previous comment.

That context menu has been added.

Oh. I never thought to look for a context menu. I can see this being a discoverability issue.

Could we have a tooltip pointing users to the context menu if they hover over the #nnn label? It wouldn't be a clickable item, but it would point them in the right direction.

@grokys

This comment has been minimized.

Copy link
Contributor

grokys commented Jul 4, 2018

Could we have a tooltip pointing users to the context menu if they hover over the #nnn label? It wouldn't be a clickable item, but it would point them in the right direction.

Perhaps, yes that might be an option. What would you suggest?

One thing to think about is that you're maybe trained that there's a clickable link there. Usually one is used to trying a right-click in VS to see more options, so it might not be necessary for new users? Of course there will be users though who are used to having a link there.

@jcansdale

This comment has been minimized.

Copy link
Collaborator

jcansdale commented Jul 5, 2018

@grokys,

Perhaps, yes that might be an option. What would you suggest?

Maybe something like: Use context menu to Open on GitHub. I'm afraid it isn't terribly elegant. 馃槩

Re #1737 (comment):

This was by design. The list is now a selectable list, and selectable lists don't generally have inner clickable items. I was thinking of adding a context menu with "Open on GitHub"?

I think my first preference would be to break this convention and allow users to click directly on the #link. If we later add support for CI status 鉁旓笍 / , we might want to allow users to click directly on the icon (just like they're used to doing on .com).

It isn't like having clickable items on a selectable list is unprecedented. Even on File Explorer there's a checkbox that can be toggled directly without using a context menu.

image

We're in an interesting position with some visual queues and ways of interacting that users will have learnt from the website.

@donokuda

This comment has been minimized.

Copy link
Member Author

donokuda commented Jul 5, 2018

As much as I understand and agree with the concerns around discoverability of the "View On GitHub" action, I'm not convinced that the pull request list is the place where we want to make that action obvious (including using a tooltip). I think a more appropriate place would be in the Pull Request Details view (which we already have in the form of a "View on GitHub" link.

If we find that folks are used to clicking the pull request number in the details list to view the PR on GitHub, then I'd like to understand what drives them to take that action at that moment:

  • Is it because they don't know that the pull request detail feature exists (e.g., a different discoverability issue in and of itself)
  • Is it because they know the details view exists, but doesn't suit their needs / workflow
  • Is it because they know the details view exists, but is hard to use or understand
  • ...etc.

I think if we have a better understanding of what drives that behavior, we can improve the extension so that action is not necessary from the pull request list view. (Side note: from my understanding, we link out to the pull request page on GitHub.com because we didn't have a details view implemented when the extension was first shipped. Therefore, viewing on GitHub.com has been more of a stop-gap.)

For a similar reason, I'm also hesitant on making CI statuses actionable from the pull request list. However, I do think it's useful to surface that information in the PR list so users can quickly identify which pull requests have failing build statuses so that they can see what's broken and fix it as quick as possible.

@donokuda

This comment has been minimized.

Copy link
Member Author

donokuda commented Jul 5, 2018

Wanted to show an update with @simurai's suggestions:

screen shot 2018-07-05 at 12 22 57 pm

This feels a little better than before. Specifically, adding some space between the comment count and timestamp helped makes a more parseable. That said, I think I want to try mocking up a few more ideas for us to review and move forward with.

@jcansdale

This comment has been minimized.

Copy link
Collaborator

jcansdale commented Jul 6, 2018

@donokuda,

That said, I wonder if having the list item separator feels out of place... Part of that is mostly because I haven't really seen examples of this type of treatment around Visual Studio.

The History in Simple View looks somewhat similar to this:

image

I actually liked the scan-ability of the comments when they're lined up on the right.

If we find that folks are used to clicking the pull request number in the details list to view the PR on GitHub, then I'd like to understand what drives them to take that action at that moment.

I do that mainly because we don't currently support general (non-inline) PR comments. It's the quickest way to see what someone has written. It also means I don't have to wait for the PR to load.

@donokuda

This comment has been minimized.

Copy link
Member Author

donokuda commented Jul 6, 2018

The History in Simple View looks somewhat similar to this:

馃槺 Thank you! This is helpful and nice to see that there is precedent elsewhere in Visual Studio 馃槃
Additionally, I presented that last iteration of the list in a design review and had some great feedback that I tried out. I ended up with this variation and feel pretty good about it:

screen shot 2018-07-06 at 2 05 40 pm

Here are a few other explorations that I tried out:

image

Out of the three, I felt like the first and second ones were stronger outcomes than the third mainly because my eye was drawn to the avatar. If we're not a huge fan of the smaller avatar treatment, then I think the first one would be a nice iteration over what exists already

@jcansdale

This comment has been minimized.

Copy link
Collaborator

jcansdale commented Jul 9, 2018

@donokuda,

If we're not a huge fan of the smaller avatar treatment, then I think the first one would be a nice iteration over what exists already

I'm leaning towards the first one. I think the smaller avatar is a bit too small.

  • There are many avatars that I can't quite work out what they are at the smaller size
  • This isn't mitigated by having the user peek view like on dotcom [1]

[1]
image

@donokuda

This comment has been minimized.

Copy link
Member Author

donokuda commented Jul 9, 2018

I'm leaning towards the first one. I think the smaller avatar is a bit too small.

Best thing I realized about exploration number 1 is that it's pretty much what I started the PR with. 馃槅

We've come full circle, folks.

Circle back to the first iteration
馃幍 It's the circle of design 馃幍

@donokuda donokuda changed the base branch from refactor/pr-list to master Jul 11, 2018

@donokuda

This comment has been minimized.

Copy link
Member Author

donokuda commented Jul 11, 2018

I ended up circling back to the first iteration to avoid moving around things unnecessarily.

Here's what the list looks like now:

screen shot 2018-07-11 at 2 16 07 pm

I updated the PR so that it targets master given that @grokys's PR was merged. This is ready for review.

donokuda and others added some commits Jul 11, 2018

@jcansdale jcansdale self-requested a review Jul 12, 2018

@jcansdale
Copy link
Collaborator

jcansdale left a comment

Shiny! 鉂囷笍 馃憤

@sguthals

This comment has been minimized.

Copy link
Contributor

sguthals commented Jul 12, 2018

One more question, which may not be easily done or preferred but I figured I'd throw it out there:

What about cutting off the title a bit more and putting the comment icon+# in the center of the two lines that we are showing?

@meaghanlewis

This comment has been minimized.

Copy link
Contributor

meaghanlewis commented Jul 12, 2018

This LGTM 馃檶 !

One thing that I liked was being able to tell which branch you have checked out with an associated PR. I saw you debated the idea of bolding the title a while back, just wondering if this was still on the table.

I removed this to keep the number of icons down in the list, but I still think it's useful to call out if you have checked out a branch with a pull request associated with it.
One idea that @grokys and I kicked around is bolding the title. This is similar to a treatment used in the "Connect" page showing which repository that you are currently in...

@sguthals

This comment has been minimized.

Copy link
Contributor

sguthals commented Jul 12, 2018

I agree with @meaghanlewis -> that would be cool to see a design of as well.

In Atom we're doing:
screen shot 2018-07-12 at 4 33 58 pm

It would be interesting to see the difference between bolding and moving it to the top. I think that it works well with Atom because that area gets replaced with more information when you aren't on a PR branch:
screen shot 2018-07-12 at 4 35 20 pm
screen shot 2018-07-12 at 4 36 11 pm

Which is different than Visual Studio. Not saying things have to be aligned, but I like exploring these different types and understanding why we make different choices (e.g. is it because of different workflows for a given editor? Different aesthetics? )

@jcansdale

This comment has been minimized.

Copy link
Collaborator

jcansdale commented Jul 13, 2018

One thing that I liked was being able to tell which branch you have checked out with an associated PR. I saw you debated the idea of bolding the title a while back, just wondering if this was still on the table.

Well spotted @meaghanlewis. I would definitely miss this functionality.

@grokys grokys merged commit aea6f73 into master Jul 13, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@grokys grokys deleted the donokuda/pr-list-polish branch Jul 13, 2018

@meaghanlewis meaghanlewis added this to the 2.5.4 milestone Jul 13, 2018

@donokuda

This comment has been minimized.

Copy link
Member Author

donokuda commented Jul 13, 2018

Which is different than Visual Studio. Not saying things have to be aligned, but I like exploring these different types and understanding why we make different choices (e.g. is it because of different workflows for a given editor? Different aesthetics? )

I explored something like that in a previous comment but punted on it to not hold up the PR. I really liked hoisting the current pull request on top and thought it worked well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.