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

Changes view: harder for me to understand "clean" state for a repo #101103

Closed
bpasero opened this issue Jun 26, 2020 · 37 comments
Closed

Changes view: harder for me to understand "clean" state for a repo #101103

bpasero opened this issue Jun 26, 2020 · 37 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders scm General SCM compound issues ux User experience issues verified Verification succeeded
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Jun 26, 2020

One thing I am not so happy about the new changes UX is that I often think that the other repos are dirty changes I need to review:

image

I wonder if there is anyway to distinguish changes (= files) more from other repos. Of course this is harder for me where file icons are turned off.

//cc @misolori

@joaomoreno joaomoreno added linux Issues with VS Code on Linux scm General SCM compound issues ux User experience issues and removed linux Issues with VS Code on Linux labels Jun 26, 2020
@joaomoreno joaomoreno added this to the June 2020 milestone Jun 26, 2020
@joaomoreno
Copy link
Member

From @gjsjohnmurray:

I don't like that the new UX no longer displays the label parameter passed on the vscode.scm.createSourceControl(...) call (e.g. Git and Deltanji). Perhaps they can be added as hovers on the Trial2 and WebTerminal headers.

👍

@gjsjohnmurray
Copy link
Contributor

In #101112 I wrote:

I like how the new UX no longer uppercases the SourceControl and SourceControlResourceGroup labels, (e.g. Trial2 and WebTerminal, Changes and #4: Fourth) but perhaps they could be bolded or get some other visual differentiation from the SourceControlResourceState entries (e.g. README2.txt and WebTerminal.Updater.CLS [WEBTERMINAL.0])

(examples cited are from screenshots in the other issue that compare 1.46 and 1.47 side by side)

@joaomoreno joaomoreno added the bug Issue identified by VS Code Team member as probable bug label Jun 29, 2020
@miguelsolorio
Copy link
Contributor

Adding the scm label I think would definitely help here:

image

@joaomoreno
Copy link
Member

I've added the SCM label.

I can't really hide the twistie if the repositories have no changes as it makes it even worse:

image

@joaomoreno
Copy link
Member

I kinda like this, will push it:

image

@joaomoreno
Copy link
Member

Even removing the twistie looks good:

image

What do you think?

joaomoreno added a commit that referenced this issue Jul 1, 2020
@bpasero
Copy link
Member Author

bpasero commented Jul 1, 2020

I would probably prefer a look that is in sync with our split views, e.g. a full border:

image

I would even think that bold could work for the first nodes?

@miguelsolorio
Copy link
Contributor

miguelsolorio commented Jul 1, 2020

I thought the solid border was fine, I don't think we ever use a dashed border so this feels inconsistent.

@gjsjohnmurray
Copy link
Contributor

How about the proposal from elsewhere to drop the badge if its value is zero?

@joaomoreno
Copy link
Member

joaomoreno commented Jul 1, 2020

@bpasero @misolori Using a full border will fool the user into thinking that it can be dragged, since now the core themes all render split views that very same way. This is something I've been trying to avoid since the beginning of this adventure. I'm just going to get an issue: can't resize SCM provider view.

I'm really all up for more ideas, keep them coming, but I don't think aligning this with splitviews is a good idea.


@gjsjohnmurray I tried that but didn't really like the fact that the actions wouldn't align with other repositories'.

@gjsjohnmurray
Copy link
Contributor

I tried that but didn't really like the fact that the actions wouldn't align with other repositories'.

@joaomoreno how about styling the badge div as visibility: hidden; ?

image

@miguelsolorio
Copy link
Contributor

Do we really need the top level badges at the repo level? Feels like the changes one is the only important one and most of the time it just repeats itself. I think this would look better too:

@bpasero
Copy link
Member Author

bpasero commented Jul 2, 2020

Throwing in another idea: use different background colors for the repos. Compare with our keybindings editor:

image

This gives a nice separation between logical units in the list. Though arguably can only be a light change in background color given we have so many selection colors.

@joaomoreno
Copy link
Member

joaomoreno commented Jul 2, 2020

@misolori Yeah I kinda like that. Though your screenshot shows the good case: you wanna see vscode-docs with no changes and vscode-internalbacklog with changes. That's the ugly edge case. I'll push the change though!

I'm also going to remove the dotted lines, they haven't grown nicely on me.


This gives a nice separation between logical units in the list. Though arguably can only be a light change in background color given we have so many selection colors.

@bpasero That's also the style of the splitview for the other half of themes:

image


@joaomoreno how about styling the badge div as visibility: hidden; ?

@gjsjohnmurray Good idea but I'll just get the bad layout in SCM view bug right after, since it looks broken.

@joaomoreno
Copy link
Member

Latest exploration:

image

joaomoreno added a commit that referenced this issue Jul 2, 2020
@bpasero bpasero added the verified Verification succeeded label Jul 2, 2020
joaomoreno added a commit that referenced this issue Jul 2, 2020
@joaomoreno
Copy link
Member

And one more iteration, now with a collapse/expand repositories action:

recording (6)

@gjsjohnmurray
Copy link
Contributor

@joaomoreno I like the Collapse All, but I see you have also dropped the top level badges entirely. So after Collapse All the only clue about which rows have changes is the presence of absence of the collapse icon at the left end.

I take your earlier point about folk reporting a suppressed 0-badge as a UI bug. How about an empty badge (i.e. no zero in it)? Or fade them?

@joaomoreno
Copy link
Member

@gjsjohnmurray I'm thinking a setting to show/hide the badge, but maybe keep the default hidden.

@gjsjohnmurray
Copy link
Contributor

@joaomoreno a setting sounds like a good compromise. Maybe tri-state, so we can "hide", "show", and "hide if zero". Default to "hide", which has the benefit of leaving a but more space for command buttons and branch name.

@joaomoreno
Copy link
Member

@gjsjohnmurray Will push a scm.providerCountBadge setting with values ['hidden', 'auto', 'visible'].

joaomoreno added a commit that referenced this issue Jul 3, 2020
gjsjohnmurray pushed a commit to gjsjohnmurray/vscode that referenced this issue Jul 3, 2020
@yume-chan
Copy link
Contributor

yume-chan commented Jul 3, 2020

I want to share my thoughts about the new design.

The "check" button used to be visible only if the corresponding repository's view is expanded, rather than showing all the time now. I wonder who would use this button without seeing what's actually changed/stage?

So I tried to move it into the view itself. From this starting point, I also came with the menu refinement ideas.

Slice 1

I also thought that maybe hovering on the counting badge will show the ... menu, so more spaces can be saved. But this may cause some confusion so I rejected this one.

@yume-chan
Copy link
Contributor

yume-chan commented Jul 3, 2020

Surprise! There are two more operations when right-clicking the header

image

Maybe these can be moved to the left of the "collapse all" button, same as the explorer.

For discoverability maybe they should also be in the header ... menu.

@gjsjohnmurray
Copy link
Contributor

@yume-chan I really like your ideas. Having multiple "..." menus solves the problem of the very long Git context menu without us having to wait for submenu support for extensions.

Maybe the menu button at (3) should be a "..." rather than an "expand down", for better consistency.

As for the surprise "Close Repository" and "Open in Terminal" context menu, maybe replicate them on menu (2). I don't think they could go alongside "collapse all" because they apply to a single repo.

@yume-chan
Copy link
Contributor

Maybe the menu button at (3) should be a "..." rather than an "expand down", for better consistency.

I want to make it a "button with menu". For example in mail apps usually there is a Reply | ▼ button. Clicking on "Reply" will do one thing, clicking the down arrow will show more operations that relates to the label. But yes I didn't see this pattern in Code.

As for the surprise "Close Repository" and "Open in Terminal" context menu, maybe replicate them on menu (2). I don't think they could go alongside "collapse all" because they apply to a single repo.

You are right. The explorer can have "New File" and "New Folder" alongside "Collapse All" is because its item can have focus, can't apply to the scm view.

@gjsjohnmurray
Copy link
Contributor

Will push a scm.providerCountBadge setting with values ['hidden', 'auto', 'visible']

@joaomoreno thanks for this, but for auto mode maybe use visibility: hidden;

image

rather than the display: none; which you implemented:

image

And I suggest default should be 'auto' so people discover the most sophisticated form of the badge feature at this level in the new UX (first screenshot above). Then if they don't like/want these badges they can find the setting and pick 'hidden'.

image

And if the white space to the right of the "..." disturbs them they can pick 'visible'.

image

@joaomoreno
Copy link
Member

@joaomoreno thanks for this, but for auto mode maybe use visibility: hidden;

I don't think that's a good idea, since we'd be wasting precious space. Especially if no repository has changes: there would just be an empty column. That is exactly the compromise with auto, otherwise we'd take it as the default.

@gjsjohnmurray
Copy link
Contributor

I don't think that's a good idea, since we'd be wasting precious space. Especially if no repository has changes: there would just be an empty column

Maybe make auto smart enough only to use visibility: hidden; if at least one other repo has changes?

Are you going to keep the default of scm.providerCountBadge as hidden? I'd still argue for defaulting to show, else some people may not realize it's even possible to show change count badges at this level.

@joaomoreno
Copy link
Member

Maybe make auto smart enough only to use visibility: hidden; if at least one other repo has changes?

This will make thing jump around in all repositories if one repository changes.

Are you going to keep the default of scm.providerCountBadge as hidden? I'd still argue for defaulting to show, else some people may not realize it's even possible to show change count badges at this level.

Yes that's the idea. The hypothesis is that most users don't even care. And the power users have the setting. Trying to stay lightweight here.

@SailorMax
Copy link

Can you add option to hide action buttons from provider line? They takes too much space +

  • Synchronize button we have in "..." menu
  • Commit button require commit description => it has to be in "Changes" subtree
  • Refresh button also has to be in "Changes" subtree, because of what we have to see after it pressed in closed provider tree?

And look at #102080 . I think it was a mistake.

thank you.

@miguelsolorio
Copy link
Contributor

Throwing out a few suggestions here:

  • What if we moved the refresh action to a global view command? This would free up a bit of space + visual noise
  • What if we only showed the commit check icon when there are changes? This would also free some space
  • What if we ellipse the repo name? You can still hover the repo to get the title but when the actions are truncated it requires more clicks
  • What if we showed the twisties for repos with no changes? We do this in the explorer with empty folders and I think could help with the hierarchy confusion

@joaomoreno
Copy link
Member

  • What if we moved the refresh action to a global view command? This would free up a bit of space + visual noise

We can't really do this since if you have many repositories, refreshing them all is a potentially expensive operation.

  • What if we only showed the commit check icon when there are changes? This would also free some space

This can be interesting yes.

  • What if we ellipse the repo name? You can still hover the repo to get the title but when the actions are truncated it requires more clicks

Yeah that will be the fix of #102108

  • What if we showed the twisties for repos with no changes? We do this in the explorer with empty folders and I think could help with the hierarchy confusion

Yeah, let me push that already.

joaomoreno added a commit that referenced this issue Jul 16, 2020
@gjsjohnmurray
Copy link
Contributor

Yeah, let me push that already.

@joaomoreno for bonus points maybe try moving the optional repo-level count badges to the start of the row instead of the end. I think having badges there will help, particularly with scm.providerCountBadge set to auto. Plus it will solve the button alignment issue auto mode gives when badges are on the right.

@gjsjohnmurray
Copy link
Contributor

With providerCountBadge set to auto and moved to the left, plus small increases to indents of the tree descendants.

image

And with the changed repo collapsed:

image

Without my tweaks:

image

and collapsed:

image

@carlocardella
Copy link
Member

Honestly, I prefer the badge count on the right. Maybe my eyes are trained to search for info on the right side but I find the repo names all properly left aligned to be more pleasing and easier to read

@joaomoreno
Copy link
Member

Yes, I'm really not convinced to move the badge to the left, since it would remove the badge alignment.

@gjsjohnmurray
Copy link
Contributor

I still think provider count badges on the left may be helpful to users who are finding it hard to spot where the changed files of one repo end and the title of the next repo starts. Personally I find the duplication of counts aligned on the right a bit unsatisfactory. When a provider section is collapsed I like to know how many changes it contains, but once I expand it and get subhead counts it's those I want to focus on, and the top-level count is distracting.

How about offering auto-left and visible-left as additional choices for scm.providerCountBadge?

On a related topic, is there any telemetry on the SCM view that would give data on how many providers users have there?

@miguelsolorio
Copy link
Contributor

We've always had badge counts on the right and moving it to the left would make it inconsistent.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders scm General SCM compound issues ux User experience issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

8 participants
@joaomoreno @bpasero @yume-chan @carlocardella @SailorMax @gjsjohnmurray @miguelsolorio and others