Add delete commands for internship and application items #140
Conversation
Codecov Report
@@ Coverage Diff @@
## master #140 +/- ##
=============================================
- Coverage 70.98% 43.05% -27.94%
- Complexity 400 416 +16
=============================================
Files 71 131 +60
Lines 1251 2137 +886
Branches 124 233 +109
=============================================
+ Hits 888 920 +32
- Misses 331 1170 +839
- Partials 32 47 +15
Continue to review full report at Codecov.
|
src/main/java/seedu/address/logic/commands/delete/DeleteInternshipCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/seedu/address/logic/parser/util/InternshipUtil.java
Outdated
Show resolved
Hide resolved
| applicationList.addItem(applicationItem); | ||
| model.setTabName(TabName.APPLICATION); |
There was a problem hiding this comment.
Is this supposed to switch the tabs? It doesn't do that at the moment.
There was a problem hiding this comment.
Not sure what is the group's consensus on this, so I took the easier path first which is to switch tabs after successful command. Could it be because the command is executed unsuccessfully? Because this should be correct since I took this from @seanjyjy switch tab command.
There was a problem hiding this comment.
okay! I tested on my add and delete commands but it doesn't do anything yet. Is there more to the switching besides setting the model state which I have missed from your implementation? I think you may have to return the multiarg CommandResult as there is a boolean arg to indicate whether to switch ?
There was a problem hiding this comment.
Just tried using the same line of code for the add company command I'm working on and yep, even upon successful addition of a company, this line of code does not switch tabs. Will follow Isaac's observation and see if I can make it work for add company in the time being!
| applicationList.addItem(applicationItem); | ||
| model.setTabName(TabName.APPLICATION); | ||
|
|
||
| return new CommandResult(String.format(MESSAGE_SUCCESS, applicationItem)); |
There was a problem hiding this comment.
Just tried successfully on my end for add company command, regarding the switching of tabs, changing this to return new CommandResult(String.format(MESSAGE_SUCCESS, applicationItem), false, false, true); will do the trick!
There was a problem hiding this comment.
I think we could just set the default value for isSwitchTab in the constructor to be true then?
There was a problem hiding this comment.
This works if all our commands require a switch? At the moment that seems to be the case apart from the help and exit commands if I'm not mistaken. Also, I suppose it depends on how we want our app to behave exactly - do we want it to switch every time a command is executed even if the user is already on the right page, or do we want to switch only if the user is on the wrong page? I suppose this may not be detectable visually unless the switching of tabs introduces its own behavior (for example if switching causes the list of cards to jump to the top of the list, and not switching allows the user to stay at wherever they've scrolled to in the list, then executing a command that always switches vs. one that only switches when necessary will provide different user experiences).
No description provided.