-
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
Milestone Picker: previous and new milestone should not be the same #1487 #1488
Milestone Picker: previous and new milestone should not be the same #1487 #1488
Conversation
0433851
to
2c8a0fc
Compare
Ready for review. |
@@ -273,6 +275,12 @@ private void addMatchingMilestones(List<PickerMilestone> milestonesToBePopulated | |||
.findFirst(); | |||
} | |||
|
|||
private List<PickerMilestone> getSelectableMilestones(List<PickerMilestone> milestones) { |
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.
Will need a header comment here. What is Selectable
and how is it defined? I didn't understand right away why is it selectable by reading the method body.
If it is difficult to write a header comment that is not tied to implementation, maybe renaming the method works better.
2c8a0fc
to
382aa81
Compare
@HansNewbie Made the changes. |
} | ||
|
||
/** | ||
* Fills the input field of milestone picker dialog with default milestone's title | ||
* Fills the input field of milestone picker dialog with best suggested milestone's name |
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.
Either use sugested
or best matching
. best suggested
sounds weird.
32aaeda
to
3814c8c
Compare
… assigned milestone. MilestonePickerDialog will simply fill its textfield with the best suggested milestone name.
…r getSelectableMilestones.
…ded test and updated comments.
3814c8c
to
de7055a
Compare
@HansNewbie Made the changes and added a test. |
Looks good. Travis failed was not related to this PR, restarted it. |
@m133225 Can merge. |
Fixes #1487