Skip to content
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

Updates to ui #188

Merged
Merged

Conversation

chrystalquek
Copy link

Fix #144 (addressbook.log -> productiv.log)
Fix #139 (addressbook -> contactbook, commandresults 'book' -> 'list')
Fix #169
Fix #132 (warning message when switching to same mode)
Fix #164 (shorten Switch commands)

@chrystalquek chrystalquek added this to the mid-v1.4 milestone Nov 2, 2020
@chrystalquek chrystalquek self-assigned this Nov 2, 2020
@chrystalquek chrystalquek added this to PRs Pending Approvals in Productiv via automation Nov 2, 2020
@codecov-io
Copy link

codecov-io commented Nov 3, 2020

Codecov Report

Merging #188 into master will increase coverage by 0.10%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #188      +/-   ##
============================================
+ Coverage     52.99%   53.09%   +0.10%     
- Complexity      742      744       +2     
============================================
  Files           166      166              
  Lines          3106     3113       +7     
  Branches        344      345       +1     
============================================
+ Hits           1646     1653       +7     
  Misses         1353     1353              
  Partials        107      107              
Impacted Files Coverage Δ Complexity Δ
src/main/java/seedu/address/MainApp.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...in/java/seedu/address/commons/core/LogsCenter.java 78.37% <ø> (ø) 10.00 <0.00> (ø)
...main/java/seedu/address/commons/core/Messages.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...dress/logic/commands/deliverable/ClearCommand.java 100.00% <ø> (ø) 2.00 <0.00> (ø)
...ress/logic/commands/deliverable/DeleteCommand.java 100.00% <ø> (ø) 7.00 <0.00> (ø)
...ddress/logic/commands/deliverable/DoneCommand.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...ddress/logic/commands/deliverable/EditCommand.java 94.66% <ø> (ø) 12.00 <0.00> (ø)
...ddress/logic/commands/deliverable/FindCommand.java 100.00% <ø> (ø) 6.00 <0.00> (ø)
...ddress/logic/commands/deliverable/ViewCommand.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...u/address/logic/commands/meeting/ClearCommand.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bff3b39...0226346. Read the comment docs.

gabztcr
gabztcr previously requested changes Nov 3, 2020
Copy link

@gabztcr gabztcr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments for your review 😎
Thanks for the great effort here 👍

@@ -801,15 +801,15 @@ testers are expected to do more *exploratory* testing.

1. Data files are saved in a `data` folder.<br>
3 JSON files are created:
* `addressbook.json`
* `contactbook.json`
* `meetingBook.json`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lower case 'b'


Format: `switch MODE`
* `switch` `deliverable`, `meeting` and `contact` mode will display your list of deliverables, meetings and contacts in the left panel respectively,
e.g. `switch contact` will display your list of contacts.
* `MODE` can be dashboard (`db`), deliverable (`dv`), meeting (`m`) or contact (`c`).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reversed information: should be "MODE can be db (dashboard), dv (deliverable), ..."

* `switch` `deliverable`, `meeting` and `contact` mode will display your list of deliverables, meetings and contacts in the left panel respectively,
e.g. `switch contact` will display your list of contacts.
* `MODE` can be dashboard (`db`), deliverable (`dv`), meeting (`m`) or contact (`c`).
* `switch` `dv`, `m` or `c` mode will display your list of deliverables, meetings and contacts in the left panel respectively,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a mental note that we may need to explain the main components in our GUI (command box, feedback box, navbar, left panel, right panel) at the start of the UG with a screenshot

@@ -18,7 +18,8 @@
public static final String COMMAND_WORD = "view";
public static final String MESSAGE_VIEW_DELIVERABLE_SUCCESS = "Viewing deliverable: %1$s";
public static final String MESSAGE_USAGE = COMMAND_WORD
+ ": Views the details of the deliverable identified by the index number used in the deliverable list.\n"
+ ": Views the details of the deliverable identified by the index number used in "
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to consider replacing "used" with "as shown"/"as seen"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original phrasing of AB3, thinking it would better to just stick to that

@@ -33,7 +33,7 @@
+ PREFIX_LOCATION + "Room 1A";

public static final String MESSAGE_SUCCESS = "New meeting added: %1$s";
public static final String MESSAGE_DUPLICATE_MEETING = "This meeting already exists in the meeting book";
public static final String MESSAGE_DUPLICATE_MEETING = "This meeting already exists in the meeting list";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing fullstop?


public static final String MESSAGE_SUCCESS = "Mode switched to: %1$s";
public static final String MESSAGE_SAME_MODE = "Switched to same mode: %s!";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using You are already in this mode! or Mode is presently displayed! so it appears more like the user made a mistake.

+ "Parameters: MODE (must be " + ModeEnum.getModeOptions()
+ ")\n"
+ "Example: " + COMMAND_WORD + " " + ModeEnum.DELIVERABLE.getArgument();


public static final String MESSAGE_SUCCESS = "Mode switched to: %1$s";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean users will get something like Mode switched to: dv? Are we able to convert into Mode switch to: deliverable (along with the other models) instead? 😨

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, its currently Mode switched to: Deliverable

@@ -34,7 +34,7 @@
+ PREFIX_DESCRIPTION + "End user";

public static final String MESSAGE_SUCCESS = "New contact added: %1$s";
public static final String MESSAGE_DUPLICATE_PERSON = "This contact already exists in the address book";
public static final String MESSAGE_DUPLICATE_PERSON = "This contact already exists in the contact list";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing exclamation mark? (Just ensure the message punctuations are consistent throughout the models)

@@ -16,9 +16,9 @@
*/
public class ViewCommand extends Command {
public static final String COMMAND_WORD = "view";
public static final String MESSAGE_VIEW_CONTACT_SUCCESS = "Viewing contact: %1$s";
public static final String MESSAGE_VIEW_PERSON_SUCCESS = "Viewing contact: %1$s";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we agreed previously that the messages should be from the perspective of the app. Here and else models, Viewing contact should be Displayed contact: %1$s.

Also, I think list command should be Listed all contacts! if it is not already. Clear should be Cleared all contacts!.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fixed by #182

Updated in that PR

Copy link

@claraadora claraadora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chrystalquek chrystalquek dismissed gabztcr’s stale review November 3, 2020 15:49

Updated according to comments.

Productiv automation moved this from PRs Pending Approvals to PRs approved / Ready for Testing Nov 3, 2020
@chrystalquek chrystalquek merged commit 3d5a9e7 into AY2021S1-CS2103T-F11-2:master Nov 3, 2020
Productiv automation moved this from PRs approved / Ready for Testing to Issues Done / PRs Merged (After Testing) Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Productiv
Issues Done / PRs Merged (After Testing)
4 participants