Skip to content

Support adding/deleting reviewers and labels - #955

Merged
Rachel Macfarlane (RMacfarlane) merged 6 commits into
masterfrom
rmacfarlane/add-reviewers
Feb 22, 2019
Merged

Rachel Macfarlane (RMacfarlane) merged 6 commits into
masterfrom
rmacfarlane/add-reviewers

Conversation

@RMacfarlane

@RMacfarlane Rachel Macfarlane (RMacfarlane) commented Feb 15, 2019 •

Copy link
Copy Markdown
Contributor

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 Rachel Macfarlane (RMacfarlane) changed the title Support adding reviewers and labels Support adding/deleting reviewers and labels Feb 16, 2019
@rebornix

Peng Lyu (rebornix) commented Feb 22, 2019 •

Copy link
Copy Markdown
Contributor

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

Comment thread src/github/pullRequestManager.ts Outdated

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

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.

do we want to await this call?

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.

yeah, nice catch

Comment thread src/github/pullRequestManager.ts Outdated

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

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.

same here.

@rebornix Peng Lyu (rebornix) left a comment

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.

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

@RMacfarlane

Copy link
Copy Markdown
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

@rebornix Peng Lyu (rebornix) left a comment

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.

:thumbs_up:

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