-
Notifications
You must be signed in to change notification settings - Fork 4
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
Visual indicator for overdue reminders and test cases for meetings and reminders commands #114
Visual indicator for overdue reminders and test cases for meetings and reminders commands #114
Conversation
# Conflicts: # src/test/java/seedu/address/logic/commands/contact/AddCommandTest.java
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.
Tested locally, LGTM! Good idea to abstract ModelStub out to another class.
Can I just check: whether a reminder is overdue or not is evaluated at program startup, are there any plans to either have the reminder display update immediately when the reminder time has passed, or to have a command to manually refresh the reminder display (like perhaps on reminder list
?
Good point! I've added a force-refresh at the end of every command. Idk if this would be too inefficient though... |
I think that should be fine, we could relook it if the refreshes cause any issues later! |
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.
Just checking: is the meeting panel refresh is implemented in this PR too?
// Force refresh of the following UI components which are time sensitive | ||
// Overdue reminders should be displayed differently | ||
this.reminderListPanel.refresh(); | ||
// Past meetings should be filtered out |
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 something that was implemented in this PR? I didn't experience any filtering out of past meetings when executing commands to refresh the display.
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.
Not yet! Just adding this in here so that I won't forget. Sorry for the confusion
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.
Great, thanks for clarifying! Will approve and merge.
Closes #110, #60
reminder list
so that it will not display the entire reminder list in the chatbox.seedu.address.commands.contact.AddCommandTest
to a public class inseedu.address.model
commands.reminder
andcommands.meeting
Demo: