Skip to content

Add new repositories to a highlighted user defined list#1900

Merged
norascheuch merged 3 commits intomainfrom
nora/add-repo-to-list
Dec 22, 2022
Merged

Add new repositories to a highlighted user defined list#1900
norascheuch merged 3 commits intomainfrom
nora/add-repo-to-list

Conversation

@norascheuch
Copy link
Copy Markdown
Contributor

When a remote user defined list is highlighted when pressing the 'Add new Database'-Button we only want to show the option to add a new repository (instead of also adding a new owner) and add the new database to the highlighted list.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@norascheuch norascheuch requested a review from a team as a code owner December 21, 2022 14:03
@norascheuch norascheuch force-pushed the nora/add-repo-to-list branch from 5f43f8c to 758c182 Compare December 21, 2022 14:23
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Great! Thanks for adding this ⚡ One comment, and I tried answering your questions (though you might want a more informed opinion 😅)

Comment on lines +87 to +89
if (highlightedItem?.kind === DbItemKind.RemoteUserDefinedList) {
await this.addNewRemoteRepo(highlightedItem.listName);
} else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From the (internal) issue:

If a list or item inside a list is highlighted, add it to that list

So I think that means something like this? (i.e. also checking if the highlighted item is inside a list)

Suggested change
if (highlightedItem?.kind === DbItemKind.RemoteUserDefinedList) {
await this.addNewRemoteRepo(highlightedItem.listName);
} else {
if (highlightedItem?.kind === DbItemKind.RemoteUserDefinedList) {
await this.addNewRemoteRepo(highlightedItem.listName);
} else if (
highlightedItem?.kind === DbItemKind.RemoteRepo &&
highlightedItem.parentListName
) {
await this.addNewRemoteRepo(highlightedItem.parentListName);
} else {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, didn't think about that! Will add the case!

Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Good stuff, just some suggestions!

Comment thread extensions/ql-vscode/src/databases/db-manager.ts Outdated
Comment thread extensions/ql-vscode/src/databases/config/db-config-store.ts Outdated
Comment thread extensions/ql-vscode/src/databases/config/db-config-store.ts Outdated
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Not sure if you were waiting for another review from me - all looks good :)

@norascheuch norascheuch merged commit 46a54a6 into main Dec 22, 2022
@norascheuch norascheuch deleted the nora/add-repo-to-list branch December 22, 2022 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants