Skip to content

Show more info in repositories list#4770

Merged
iAmWillShepherd merged 40 commits intomasterfrom
more-repo-info
Jul 2, 2018
Merged

Show more info in repositories list#4770
iAmWillShepherd merged 40 commits intomasterfrom
more-repo-info

Conversation

@vanessayuenn
Copy link
Contributor

@vanessayuenn vanessayuenn commented May 23, 2018

kapture 2018-05-23 at 18 33 38

Hi all! This PR adds some indicators on the repositories list view to provide more context & information (whether there are uncommited changes, and/or unsynced upstream/local commits) about the respective repo. This is in response to the issues raised by various users as summarized in the roll-up issue (resolves #2259).

All & any feedback is welcome, but there are a few things I specifically would like to hear your thoughts on:

  1. Right now, the repo info gets fetched in the background every 5 minutes, which is entirely arbituary. Any concerns about this frequency?
  2. I was told it's on the roadmap that the blue dot indicator at the commit tab will go away, which will then make the blue dot in the repo list non-reflective of the rest of the UI. (cc @donokuda 👀)
  3. I like @shiftkey's idea about using tooltip to complement this feature, so I tried it out. But as you can see from the screenshot below, it gets quite crowded and so I'm unsure about it.

screen shot 2018-05-23 at 18 37 26

@vanessayuenn vanessayuenn added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 23, 2018
@donokuda
Copy link
Contributor

donokuda commented May 23, 2018

🙌:sparkles: Really liking how this came out!

I was told it's on the roadmap that the blue dot indicator at the commit tab will go away, which will then make the blue dot in the repo list non-reflective of the rest of the UI. (cc @donokuda 👀)

I'm not too concerned about this. For what it's worth, the blue dot is ambiguous enough that it could mean almost anything; and in fact, I'd love to hear your thoughts on having a tooltip for the blue dot that says something along the lines of "There are uncommitted changes in this repository"

I like @shiftkey's idea about using tooltip to complement this feature, so I tried it out. But as you can see from the screenshot below, it gets quite crowded and so I'm unsure about it.

🙈Yup, that looks pretty crowded! I wonder we can condense the text down to something like "The currently checked out branch is commits behind [and/or ahead] of its tracked branch"


I chatted with @ampinsk and she thought of a couple of ways to improve the notification dot (which I pushed to a branch named more-repo-info-ui-tweaks). I ended up settling on this in case you wanted to bring those changes into your branch and would love to hear your thoughts on this:

image

Here's what changed:

  • Moved the notification dot to the left of the repo icon / name. This makes it easier to scan for the blue notification dot (it previously felt like it was getting lost in the repo octicon)
  • Lightened the behind/ahead indicator. The indicator seemed to be visually heavy that my eyes wanted to gravitate towards the right side of the panel. Making the indicator a little lighter and smaller helped alleviate that.

There's a few styling issues happening (specifically when the background color of the list item is blue), but I can address those myself and wouldn't consider those blockers to this PR

},
{ timeout: ReadyDelay }
)
const interval = 1000 * 60 * 5

This comment was marked as spam.

targetRepo?: Repository
): Promise<ReadonlyArray<Repository>> {
for (const repo of repositories) {
if (!targetRepo || targetRepo.id === repo.id) {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

if (!targetRepo || targetRepo.id === repo.id) {
await this.withAuthenticatingUser(repo, async (repo, account) => {
const gitStore = this.getGitStore(repo)
repo.setAheadBehind(gitStore.aheadBehind)

This comment was marked as spam.

this.repositories = repositories
this.repositories = await this.repositoriesStore
.getAll()
.then(repos => this.getRefreshedRepositories(repos))

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

repo.setAheadBehind(gitStore.aheadBehind)
await gitStore.fetch(account, true)
const status = await gitStore.loadStatus()
if (status) {

This comment was marked as spam.

@vanessayuenn
Copy link
Contributor Author

vanessayuenn commented May 25, 2018

@donokuda @ampinsk I love the tweaks you made! Pulled them into this working branch already. :)

I'd love to hear your thoughts on having a tooltip for the blue dot that says something along the lines of "There are uncommitted changes in this repository"

I wonder we can condense the text down to something like "The currently checked out branch is commits behind [and/or ahead] of its tracked branch"

Instead of a catch-all tooltip, I experimented with splitting it up into three tooltips: 1 for the blue dot, 1 for repo name (what we currently have), and 1 for the arrows. Hopefully that will be less crowded and will clarify what each indicator means. What do you think?

screen shot 2018-05-25 at 16 03 49

screen shot 2018-05-25 at 16 04 03

screen shot 2018-05-25 at 16 03 58

@donokuda
Copy link
Contributor

Instead of a catch-all tooltip, I experimented with splitting it up into three tooltips: 1 for the blue dot, 1 for repo name (what we currently have), and 1 for the arrows. Hopefully that will be less crowded and will clarify what each indicator means. What do you think?

Sorry, this comment slipped under my radar 🙈, but I like this approach! 👍

@shiftkey
Copy link
Member

shiftkey commented Jun 6, 2018

@vanessayuenn we were chatting about this PR yesterday in Slack but here's the two other things to think about aside from the shape of the app state that we've been wrestling with:

  • feature-flagging - are we able to control how we roll this feature out gradually to users, just in case we encounter some unexpected issues in the wild?
  • performance/load testing - currently we're doing a fetch for each repository in the list every five minutes. We already have a request to disable background fetching in Option to disable auto-fetching (background fetching) in Desktop #1128, so I'm curious what the risks are of introducing more background work like this, and how we might manage this for users who are tracking a significant number of repositories that are active (meaning fetch is often necessary to keep up).

Copy link
Contributor

@iAmWillShepherd iAmWillShepherd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question

{ timeout: ReadyDelay }
)

window.setInterval(() => {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

}))
}

public recordRepoClicked(repoHasIndicator: boolean): Promise<void> {

This comment was marked as spam.

@iAmWillShepherd iAmWillShepherd self-assigned this Jun 27, 2018
@shiftkey shiftkey modified the milestones: 1.2.1, 1.2.x Jun 27, 2018
@iAmWillShepherd iAmWillShepherd merged commit 1cd48a0 into master Jul 2, 2018
@iAmWillShepherd iAmWillShepherd deleted the more-repo-info branch July 2, 2018 14:34
@lock lock bot locked as resolved and limited conversation to collaborators Aug 31, 2018
@desktop desktop unlocked this conversation Sep 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants