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

Support adding/deleting reviewers and labels #955

Merged
merged 6 commits into from
Feb 22, 2019

Conversation

RMacfarlane
Copy link
Contributor

@RMacfarlane RMacfarlane commented Feb 15, 2019

Builds on top of #951
#412

Adding uses the quickpick:
add_reviewer

Icon to remove appears on hover when the view is shown as a sidebar:
delete_reviewer

When the window is less than 900px wide, content looks like, with delete icons always shown:
screen shot 2019-02-15 at 4 08 07 pm

@RMacfarlane RMacfarlane changed the title Support adding reviewers and labels Support adding/deleting reviewers and labels Feb 16, 2019
@rebornix
Copy link
Member

rebornix commented Feb 22, 2019

Run into following issues

  • Open PR description of Update README auchenberg/pullrequest-demo#45
  • Try to add a label
  • Labels already assigned are not shown on the list
  • Add one (say question)
  • question is added successfully
  • Click the + icon again, question is still on the list.

Quickpick related

  • Open PR description of Update README auchenberg/pullrequest-demo#45
  • Split the editor
  • Click the + icon to add a label, nothing happens (I think the quick pick shows up and dismisses again)
  • Repeat the steps again and this time try to remove a label, it works as expected


async addLabels(pullRequest: PullRequestModel, labels: string[]): Promise<void> {
const { octokit, remote } = await pullRequest.githubRepository.ensure();
octokit.issues.addLabels({
Copy link
Member

Choose a reason for hiding this comment

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

do we want to await this call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, nice catch


async removeLabel(pullRequest: PullRequestModel, label: string): Promise<void> {
const { octokit, remote } = await pullRequest.githubRepository.ensure();
octokit.issues.removeLabel({
Copy link
Member

Choose a reason for hiding this comment

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

same here.

Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

LGTM in overall, the label filtering issue is probably what we want to fix before merge.

@RMacfarlane
Copy link
Contributor Author

Fixed the label filtering problem, the quickpick issue seems to be a problem with the webview getting refocused, so I think it might be an upstream issue

Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

:thumbs_up:

@RMacfarlane RMacfarlane merged commit 8de03cc into master Feb 22, 2019
@RMacfarlane RMacfarlane deleted the rmacfarlane/add-reviewers branch March 7, 2019 00:24
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.

2 participants