-
Notifications
You must be signed in to change notification settings - Fork 33
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
Create a Assignee Picker that allows the user to add/remove/modify the assignee of an issue #1288 #1303
Conversation
b103133
to
45bb8c3
Compare
Change status when ready for review. Also, post a screenshot. The interaction should be similar to label picker (i.e. type to select). The avatar can be shown bigger. |
06b2d22
to
548cd2a
Compare
4e2177d
to
f05c2b6
Compare
1a20f31
to
2e7e86d
Compare
return true; | ||
} | ||
|
||
//TODO yy change remove isUpdateSuccessful from log msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO supposed to be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. I will remove it.
313662d
to
61a2c10
Compare
@ndt93 this issue needs a reviewer. @garbanzos please ask for a reviewer if you don't get one within 24 hours after it is ready for review. |
@dariusf Can you help review this? Thanks. |
private void showAssigneePicker(TurboIssue issue) { | ||
List<TurboUser> assigneeList = ui.logic.getRepo(issue.getRepoId()).getUsers(); | ||
AssigneePickerDialog assigneePickerDialog = new AssigneePickerDialog(stage, issue, assigneeList); | ||
Optional<AssigneePickerDialogResponse> assigneeDialogResponse = assigneePickerDialog.showAndWait(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I said the same things for the milestone picker:
- Types are implementation details, so we can leave them out of variable names.
assignees
? - We don't really need to name the intermediate result, so we can do
new AssigneePickerDialog(...).showAndWait(...)
.
Also, can the dialog response by an inner class? Then we don't have to prefix it.
- intial semi-working version of assignee picker - UI and interactions of assignee picker attempts to mimic the label picker
…gneePickerTests for assignee picker
- Scroll pane displays message when there are no assignees that match query - Added resetList & fixed problem that existing assignee is not showing - Change style of label and padding of flowpane
c193684
to
ac81456
Compare
return issue.getAssignee().get().equals(assignee.getLoginName()); | ||
} | ||
|
||
private void setupButtons(DialogPane assigneePickerDialogPane) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this a little less verbose, we can maybe leave out the assigneePicker
prefix, since it's clear from the context in which this method appears.
The avatar taking a long time to load could be made a separate issue. |
@dariusf ready for review again. |
import java.util.stream.Stream; | ||
|
||
public class AssigneePickerState { | ||
private List<PickerAssignee> currentAssigneesList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the better name is ...UsersList
or ...MembersList
? There can be only one assignee but there can be a list of users. Create a separate issue to change Assignee to a more appropriate name (Member/User) across all this code.
…ith spaces - Assignee picker related backend methods accept Optional<String> as argument - Refactored isFaded to isMatching and removed isHighlighted in PickerAssignee - Added AssigneePickerDialogResponse - Added removeAssignee in TurboIssue - Tweaked UI of assignee picker
- Changed context menu text for assignee picker menu item - Removed test case of 'a' keyboard shortcut from chromedrivertest - Added GUI tests for assignee picker
- Moved getExistingAssignee and getSelectedAssignee to PickerAssignee - Moved AssigneePickerTests from unstable to guitests - AssigneePickerDialogResponse is now an inner class of AssigneePickerDialog
5ebd3ee
to
e0792d2
Compare
Fixes #1288