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

Allow creating an alias for repositories #12000

Merged
merged 8 commits into from
Apr 15, 2021
Merged

Conversation

sergiou87
Copy link
Member

This is part of #7856

Description

This PR adds initial support to create alias for repositories:

  • New context menu actions to create, change or remove the alias of a repository from the repository list. Any character is valid.
  • Filtering repositories now takes the alias into account.
  • The original repository name is still visible by hovering over it, in the tooltip.

Screenshots

repo-alias-demo.mov

Release notes

Notes: [New] Add an alias to your repos and name them however you want!

Comment on lines +1454 to +1459
// No need to update the recent repositories if the selected repository is
// the same as the old one (this could happen when the alias of the selected
// repository is changed).
if (previousRepositoryId === currentRepositoryId) {
return
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, creating/changing/removing the alias of the current repo, would move the current repo into the Recents list, which doesn't make sense.

@@ -132,6 +133,7 @@ export class RepositoriesStore extends TypedBaseStore<
? await this.findGitHubRepositoryByID(repo.gitHubRepositoryID)
: await Promise.resolve(null), // Dexie gets confused if we return null
repo.missing,
enableRepositoryAliases() ? repo.alias : null,
Copy link
Member Author

Choose a reason for hiding this comment

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

This prevents the alias from being loaded from persistence, so we can forget about checking the feature flag in many other places 😅

icon = iconForRepository(repository)
title = repository.name
title = alias ?? repository.name
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the title of the drop down that shows the current repository.

@@ -89,7 +89,7 @@ export function groupRepositories(
const { aheadBehind, changedFilesCount } =
localRepositoryStateLookup.get(r.id) || fallbackValue
const repositoryText =
r instanceof Repository ? [r.name, nameOf(r)] : [r.name]
r instanceof Repository ? [r.alias ?? r.name, nameOf(r)] : [r.name]
Copy link
Member Author

Choose a reason for hiding this comment

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

This basically makes sure the alias is used to look for matches when the user filters the repository list.

const repositoryText =
repository instanceof Repository
? [repository.name, nameOf(repository)]
? [repositoryAlias ?? repository.name, nameOf(repository)]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also for the repository list filter.

Comment on lines -135 to +138
const existingCount = names.get(repository.name) || 0
names.set(repository.name, existingCount + 1)
const alias = repository instanceof Repository ? repository.alias : null
const name = alias ?? repository.name
const existingCount = names.get(name) || 0
names.set(name, existingCount + 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

This list of names is used to detect duplicates in the Recents group, to know when we need to disambiguate showing the owner as a prefix (e.g. github/desktop).
In this change I'm just adding the alias instead of the repo name, when available.

Comment on lines -154 to +158
const nameCount = names.get(repository.name) || 0
const nameCount = names.get(repositoryAlias ?? repository.name) || 0
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where we actually check if the repo alias or name is duplicated and need disambiguation.

@sergiou87 sergiou87 marked this pull request as ready for review April 14, 2021 16:08
@sergiou87 sergiou87 requested a review from tidy-dev April 14, 2021 16:08
private buildAliasMenuItems(): ReadonlyArray<IMenuItem> {
const repository = this.props.repository

if (!(repository instanceof Repository) || !enableRepositoryAliases()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious.. is there a time that it is not an instance of a Repository?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm.. just realized there is a type of Repositoryish. Which made me laugh... But I see that evidently it can be cloning one.

Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

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

✨ This is awesome! Though it is a pretty straight forward PR. I really appreciate all your PR comments along the way - makes reading through it a breeze. Tested it out and it is slick.

Only thing I came across which I don't think causes any harm - just a UI quirk. I can make the alias as long as I want, but I can only ever see so many characters and when I saw the ellipsis, I automatically tried to hover over it to see the rest of the alias. This likely not something people will do tho.. and if they do.. they can change it. 🤷

image

@sergiou87
Copy link
Member Author

Thank you for your feedback! ❤️

The thing about the long aliases and not being able to see it in the tooltip is a good point, but also true that they can just change it 😆  I'll raise it in the next sync meeting.

@sergiou87 sergiou87 merged commit 37d162d into development Apr 15, 2021
@sergiou87 sergiou87 deleted the repository-alias branch April 15, 2021 07:55
@sergiou87 sergiou87 linked an issue Apr 15, 2021 that may be closed by this pull request
@wshaver
Copy link

wshaver commented Apr 16, 2021

Thank you thank you thank you thank you <3<3<3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename repository in the UI (or via a Workaround)
3 participants