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

[v1.1][F09-B1]Your TA #80

Open
wants to merge 506 commits into
base: master
Choose a base branch
from

Conversation

WoodySIN
Copy link

@WoodySIN WoodySIN commented Mar 9, 2018

No description provided.

Copy link

@RonakLakhotia RonakLakhotia left a comment

Choose a reason for hiding this comment

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

Take note of coding standards

|`* *` |user |hide <<private-contact-detail,private contact details>> by default |minimize chance of someone else seeing them by accident

|`*` |user with many persons in the address book |sort persons by name |locate a person easily
|`* *` |user |import students/people from a text file |it is easier to enter large numbers of people

Choose a reason for hiding this comment

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

Make sure for your user stories you are in sync with your target profile.

@@ -79,4 +79,9 @@ public void saveAddressBook(ReadOnlyAddressBook addressBook, String filePath) th
XmlFileStorage.saveDataToFile(file, new XmlSerializableAddressBook(addressBook));
}

@Override
public void backupAddressBook(ReadOnlyAddressBook addressBook) throws IOException {
saveAddressBook(addressBook, (filePath + ".bak"));

Choose a reason for hiding this comment

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

Save the extension in a variable

@WoodySIN WoodySIN changed the title [v1.0][F09-B1]Your TA [v1.1][F09-B1]Your TA Mar 16, 2018
Copy link

@RonakLakhotia RonakLakhotia left a comment

Choose a reason for hiding this comment

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

Overall, good work done. Keep updating the UG/DG as you implement features.

@@ -0,0 +1,292 @@
= Your TA - User Guide

Choose a reason for hiding this comment

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

What is the purpose of this file?

Alias: `e` +
Format: `edit INDEX [n/NAME] [p/PHONE] [e/EMAIL] [a/ADDRESS] [t/TAG]...`

****

Choose a reason for hiding this comment

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

Remove text that you do not need

@@ -9,5 +9,7 @@
public static final String MESSAGE_INVALID_COMMAND_FORMAT = "Invalid command format! \n%1$s";
public static final String MESSAGE_INVALID_PERSON_DISPLAYED_INDEX = "The person index provided is invalid";
public static final String MESSAGE_PERSONS_LISTED_OVERVIEW = "%1$d persons listed!";
public static final String MAIL_APP_ERROR = "Error opening the default mail app on this system";

Choose a reason for hiding this comment

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

Follow the variable naming standard used above. Use names like MESSAGE_MAIL_APP_ERROR

String firstCharacter = test.substring(0, 1);
String nextCharacters = test.substring(1, test.length() - 1);
String lastCharacter = test.substring(test.length() - 1, test.length());
return firstCharacter.matches(MATRIC_NUMBER_VALIDATION_REGEX_FIRST)

Choose a reason for hiding this comment

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

Break up the return statement conditions in two or three steps

whenzei added a commit to whenzei/main that referenced this pull request Mar 27, 2018
wynonaK pushed a commit to wynonaK/addressbook-level4 that referenced this pull request Mar 29, 2018
Copy link

@RonakLakhotia RonakLakhotia left a comment

Choose a reason for hiding this comment

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

Make sure UG/DG are updated time to time with all relevant changes.

JoonKai1995 and others added 16 commits April 11, 2018 18:49
Added a toggle feature to the application. Toggle is located on the menu, and pressing toggle toggles between a light and dark theme.
Add integration tests for UpdateDisplayCommand
Add comment to mention why files not used
User guide & developer guide for Theme toggle , Task and Calendar features.

Changed certain feature message to user to be clearer.
Alaru and others added 30 commits April 15, 2018 21:45
Documentation: Update UG/DG broken links and rephrased some portions.
Updated figure references as well.
Documentation: Update relevant DG portion
Edit error message for markPart
# Conflicts:
#	collated/functional/Alaru-reused.md
#	collated/functional/JoonKai1995.md
#	collated/test/Alaru.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants