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
Ui for Plan #72
Ui for Plan #72
Conversation
Pull Request Test Coverage Report for Build 173
💛 - Coveralls |
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.
Good job! Can consider writing some tests for the features as well.
* As a consequence, UI elements' variable names cannot be set to such keywords | ||
* or an exception will be thrown by JavaFX during runtime. | ||
* | ||
* @see <a href="https://github.com/se-edu/addressbook-level4/issues/336">The issue on AddressBook level 4</a> |
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 realize this comment can be removed
|
||
@Override | ||
public boolean equals(Object other) { | ||
// short circuit if same object |
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.
Try to avoid using comments that describe 'how'. They should be used for describing 'why'
endDate.setText(plan.getEndDate().format(Plan.FORMATTER)); | ||
plan.getTasks().stream() | ||
.sorted(Comparator.comparing(Task::getDateTime)) | ||
.forEach(task -> tasks.getChildren().add(new Label(task.getProblem().toString() + '\n' |
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.
Could abstract this out into a formatter
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.
LGTM
No description provided.