New UI, refactoring and small fixes #86

jianhandev · 2020-10-06T11:56:36Z

UI Changes:

Changed UI theme to white
New layout (tab pane -> dashboard, patient, appointment tabs etc)
Automatic tab switching upon entering command

Logic component changes:

Refactor classes related to patient into a separate patient package

Small fixes:

Style issues (e.g. import statements, javadoc)
Bug fixes

Note: Added an "dummy" appointment class (for testing the rest of the app) - to be removed.

Other changes: make remark optional in AddCommandParser

kaitlynng

Looks good, mostly need to change command names and clarify a bit more on TAB_ID. Would be good if there's another reviewer too.

kaitlynng · 2020-10-06T14:21:32Z

src/main/java/seedu/address/logic/commands/Command.java

@@ -8,6 +9,8 @@
 */
 public abstract class Command {

+    public static final Index TAB_ID = Index.fromOneBased(3);


Maybe we can have an enum for TAB_ID instead to make it more clear.

Sorry, can you explain what is the TAB_ID used for in the Command class actually? Is it only relevant in the CommandResult class?

kaitlynng · 2020-10-06T14:22:12Z

src/main/java/seedu/address/logic/commands/CommandResult.java

@@ -17,21 +19,25 @@
    /** The application should exit. */
    private final boolean exit;

+    /** Switch to the tab indicated by tabId */
+    private final Index tabId;


Will it be confusing to have TAB_ID in Command.java and tabId in CommandResult.java? Maybe more descriptive name like tabIdToSwitchTo can help.

kaitlynng · 2020-10-06T14:22:50Z

src/main/java/seedu/address/logic/commands/patient/AddPatientCommand.java

 import seedu.address.logic.commands.exceptions.CommandException;
 import seedu.address.model.Model;
 import seedu.address.model.person.Person;

 /**
 * Adds a person to the address book.
 */
-public class AddCommand extends Command {
+public class AddPatientCommand extends Command {

    public static final String COMMAND_WORD = "add";


Should the command word be 'add' or 'add patient'? Since we've changed the AddCommand to AddPatientCommand, and we need to consider adding appointments too.

Agreed! I think it will be addpatient based on our user guide.

kaitlynng · 2020-10-06T14:23:25Z

src/main/java/seedu/address/logic/commands/patient/AddPatientCommand.java

@@ -41,7 +43,7 @@
    /**
     * Creates an AddCommand to add the specified {@code Person}
     */
-    public AddCommand(Person person) {
+    public AddPatientCommand(Person person) {


Should rename 'Person' to 'Patient' (but I think this is model people's job)

Good point, we'll do that in the next PR!

kaitlynng · 2020-10-06T14:23:46Z

src/main/java/seedu/address/logic/commands/patient/ClearCommand.java

@@ -13,11 +16,10 @@
    public static final String COMMAND_WORD = "clear";


Since AddCommand was changed to AddPatientCommand, maybe ClearCommand should be changed to be more descriptive also? i.e. change to ClearAllPatientsCommand.

kaitlynng · 2020-10-06T14:28:13Z

src/main/java/seedu/address/logic/commands/patient/EditCommand.java

+import seedu.address.model.person.Name;
+import seedu.address.model.person.Person;
+import seedu.address.model.person.Phone;
+import seedu.address.model.person.Remark;


I can't add a comment at the required line, but maybe we can rename this command to EditPatientCommand instead to align with AddPatientCommand? Also because we are probably adding capabilities or editing appointments right, so will need to clarify what we're editing.

Will also need to remember to edit the MESSAGE_USAGE to reflect patient parameters.

kaitlynng · 2020-10-06T14:30:02Z

src/main/java/seedu/address/logic/commands/patient/FindCommand.java


 import static java.util.Objects.requireNonNull;

 import seedu.address.commons.core.Messages;
+import seedu.address.logic.commands.Command;
+import seedu.address.logic.commands.CommandResult;


Likewise, should we rename to FindPatientCommand?

kaitlynng · 2020-10-06T14:31:32Z

src/main/java/seedu/address/logic/commands/patient/ListCommand.java


 import static java.util.Objects.requireNonNull;
 import static seedu.address.model.Model.PREDICATE_SHOW_ALL_PERSONS;

+import seedu.address.commons.core.index.Index;
+import seedu.address.logic.commands.Command;
+import seedu.address.logic.commands.CommandResult;


Likewise, should we change this to ListPatientsCommand? I'm not very sure because then now we'll have the 'Patient' in every command which is a lot more verbose, but it also makes the command clearer.

I think so, because we will have various ListAppointmentsCommands, like listing all appointment of a specific patient!

kaitlynng · 2020-10-06T14:38:54Z

src/main/java/seedu/address/logic/parser/patient/AddCommandParser.java

        ArgumentMultimap argMultimap =
                ArgumentTokenizer.tokenize(args, PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_ADDRESS, PREFIX_TAG,
                        PREFIX_REMARK);

-        if (!arePrefixesPresent(argMultimap, PREFIX_NAME, PREFIX_ADDRESS, PREFIX_PHONE, PREFIX_EMAIL, PREFIX_REMARK)
+        if (!arePrefixesPresent(argMultimap, PREFIX_NAME, PREFIX_ADDRESS, PREFIX_PHONE, PREFIX_EMAIL)


Why is PREFIX_REMARK removed?

kaitlynng · 2020-10-06T14:45:49Z

src/main/java/seedu/address/logic/commands/patient/RemarkCommand.java


 import seedu.address.commons.core.index.Index;
+import seedu.address.logic.commands.Command;
+import seedu.address.logic.commands.CommandResult;


Maybe we should have RemarkCommand for both patient and appointment? so we split into RemarkPatientCommand and RemarkAppointmentCommand

Appointment class already has a String description, is that good enough?

porkeypine · 2020-10-06T18:00:13Z

src/main/java/seedu/address/logic/commands/ExitCommand.java

@@ -13,7 +14,7 @@

    @Override
    public CommandResult execute(Model model) {
-        return new CommandResult(MESSAGE_EXIT_ACKNOWLEDGEMENT, false, true);
+        return new CommandResult(MESSAGE_EXIT_ACKNOWLEDGEMENT, false, true, TAB_ID);


TAB_ID needs to be declared as a public static final constant above right?

jianhandev added 5 commits October 4, 2020 19:01

Implement new UI layout and add tabs for each feature

a104d45

Add dummy classes for temporary use (testing of UI)

5bae6be

Add ability to switch tabs

78cc8d2

Other changes: make remark optional in AddCommandParser

Refactor commands related to patient into new patient package

7bffb07

Refactor parser classes related to patient into new patient package

8a49580

jianhandev added this to the v1.2 milestone Oct 6, 2020

jianhandev requested review from kaitlynng, porkeypine, ReubenTeng and thutahw October 6, 2020 11:56

jianhandev self-assigned this Oct 6, 2020

jianhandev linked an issue Oct 6, 2020 that may be closed by this pull request

Change UI layout and give the application a white interface #84

Closed

AY2021S1-CS2103T-W12-3 deleted a comment from jianhandev Oct 6, 2020

kaitlynng requested changes Oct 6, 2020

View reviewed changes

porkeypine reviewed Oct 6, 2020

View reviewed changes

kaitlynng closed this Oct 7, 2020

kaitlynng reopened this Oct 7, 2020

jianhandev closed this Oct 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New UI, refactoring and small fixes #86

New UI, refactoring and small fixes #86

jianhandev commented Oct 6, 2020

kaitlynng left a comment

kaitlynng Oct 6, 2020

porkeypine Oct 6, 2020

kaitlynng Oct 6, 2020

kaitlynng Oct 6, 2020

porkeypine Oct 6, 2020 •

edited

kaitlynng Oct 6, 2020

porkeypine Oct 6, 2020

kaitlynng Oct 6, 2020

porkeypine Oct 6, 2020

kaitlynng Oct 6, 2020

kaitlynng Oct 6, 2020

porkeypine Oct 6, 2020

kaitlynng Oct 6, 2020

kaitlynng Oct 6, 2020

porkeypine Oct 6, 2020 •

edited

kaitlynng Oct 6, 2020

kaitlynng Oct 6, 2020

porkeypine Oct 6, 2020

porkeypine Oct 6, 2020

		@@ -13,11 +16,10 @@
		public static final String COMMAND_WORD = "clear";

New UI, refactoring and small fixes #86

New UI, refactoring and small fixes #86

Conversation

jianhandev commented Oct 6, 2020

kaitlynng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porkeypine Oct 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porkeypine Oct 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porkeypine Oct 6, 2020 •

edited

porkeypine Oct 6, 2020 •

edited