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.5][T09-B2] CollegeZone #47

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

Conversation

deborahlow97
Copy link

No description provided.

@nus-se-pr-bot nus-se-pr-bot assigned dalessr and unassigned sooyj Mar 8, 2018
@deborahlow97 deborahlow97 changed the title [v1.0][T09-B2] Friendzone [v1.0][T09-B2] CollegeZone Mar 9, 2018
Copy link

@dalessr dalessr left a comment

Choose a reason for hiding this comment

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

@deborahlow97 @zuweitrack @fuadsahmawi @A0158738X Some comments added. Can make changes accordingly.

@@ -782,6 +782,7 @@ See this https://github.com/se-edu/addressbook-level4/pull/599[PR] for the step-

*Target user profile*:

* NUS Students living in RC
Copy link

Choose a reason for hiding this comment

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

Is it better to write as Residential College (RC) since other developers might not know what RC stands for?


|`* * *` |user |set a level of friendship with a specific person |maintain my friendships depending on a priority system set by myself

|`* * *` |user |edit details of my contacts |stay updated with the current details of my friends
Copy link

Choose a reason for hiding this comment

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

Here user should be replaced by your specified target users.


|`* * *` |user |edit details of my contacts |stay updated with the current details of my friends

|`* * *` |forgetful RC student |to add persistent reminders |periodically remind myself to do something.
Copy link

Choose a reason for hiding this comment

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

Second column inconsistent with the other user stories?


|`* *` |careless RC student |undo a command I entered |undo a wrong command that I entered

|`* *` |careless RC student |redo a command I entered |redo when I want to undo my "undo" command
Copy link

Choose a reason for hiding this comment

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

No need to repeat the features that current version of addressbook already contains.

@@ -852,6 +909,10 @@ _{More to be added}_
. Should work on any <<mainstream-os,mainstream OS>> as long as it has Java `1.8.0_60` or higher installed.
. Should be able to hold up to 1000 persons without a noticeable sluggishness in performance for typical usage.
. A user with above average typing speed for regular English text (i.e. not code, not system admin commands) should be able to accomplish most of the tasks faster using commands than using the mouse.
. Should be able to deal with invalid command inputs.
Copy link

Choose a reason for hiding this comment

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

Is this an NFR or a functional requirement?

@deborahlow97 deborahlow97 changed the title [v1.0][T09-B2] CollegeZone [v1.1][T09-B2] CollegeZone Mar 17, 2018
Copy link

@dalessr dalessr left a comment

Choose a reason for hiding this comment

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

@deborahlow97 @fuadsahmawi @zuweitrack @A0158738X some comments added.

Please note that:

  • Update the developer guide to describe the new features added (in Implementation part)

README.adoc Outdated
** Support for _Build Automation_ using Gradle and for _Continuous Integration_ using Travis CI.
* CollegeZone is a desktop application for *NUS Residential College 4 (RC4) students*. It has a GUI but most of the user interactions happen using a CLI (Command Line Interface).
* It's created for a RC4 resident to manage their contacts with other RC4 residents and to manage their tasks.

Copy link

Choose a reason for hiding this comment

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

Remember to add acknowledgement for CS2103 AB4 (SE-EDU)

@@ -51,27 +51,51 @@ public Command parseCommand(String userInput) throws ParseException {
case AddCommand.COMMAND_WORD:
return new AddCommandParser().parse(arguments);

case AddCommand.COMMAND_ALIAS:
Copy link

Choose a reason for hiding this comment

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

You can actually combine the command word and command alias together for the same command to make it clearer

private static final String FXML = "PersonListCard.fxml";
private static final String[] TAG_COLOR_STYLES = new String[]{"teal", "red", "yellow", "blue", "orange", "brown",
Copy link

Choose a reason for hiding this comment

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

Just a suggestion: try to use the colors which users can see the word inside the tag clearly

@@ -68,4 +68,13 @@ public String getEmail() {
.map(Label::getText)
.collect(Collectors.toList());
}

public List<String> getTagStyleClasses(String tag) {
return tagLabels
Copy link

Choose a reason for hiding this comment

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

Here there should be one component after tagLabels

*/
private FindCommand prepareTagCommand(String userInput) {
FindCommand command =
new FindCommand(new TagContainsKeywordsPredicate(Arrays.asList(userInput.split("\\s+"))));
Copy link

Choose a reason for hiding this comment

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

Incorrect format (not following the coding standard)

@dalessr
Copy link

dalessr commented Mar 18, 2018

@deborahlow97 @fuadsahmawi @zuweitrack @A0158738X
There is one thing I forgot to mention:

  • Think about the major and minor features you want to implement, and update your user guide to show all the new features (commands), even though you haven't started yet!

whenzei added a commit to whenzei/main that referenced this pull request Mar 18, 2018
…2103-AY1718S2#47)

* DeveloperGuide.adoc: add new subsection Feature Contributions

* Fix capitalization of word
@dalessr
Copy link

dalessr commented Mar 25, 2018

@deborahlow97 @fuadsahmawi @zuweitrack @A0158738X Good job 👍

Here are something you need to take note:

  • Change the PR title (v1.x) before each tutorial
  • I have seen some coming in v2.0 labels inside user guide, remember to put in the enhancements you have done and label them as since v1.x. All the other original features can also be labeled as since v1.0 to make them consistent. Try to complete your user guide by the next tutorial
  • User guide can include screenshots about how to use the command and what is the expected outcome
  • Update FAQ and Command Summary in user guide accordingly
  • Inside developer guide, update the UML diagrams accordingly since you have made changes on the project structure. Include sequence diagrams for the new features inside Implementation
  • Add more labels inside the issue tracker (i.e. priority, status), and assign the tasks to different persons
  • For the issues labeled as story, the benefit part (after so that) should be put inside the description of this issue instead of title
  • Make sure all the PRs are linked to the corresponding issues using the issue number
  • Remember to close the milestone before each tutorial. I saw that the v1.2 milestone is still open
  • Remember to add acknowledgements for Addressbook Level 4 inside REAMDE.md. You can refer to other team' repo
  • Write your project portfolio

@deborahlow97 deborahlow97 changed the title [v1.1][T09-B2] CollegeZone [v1.3][T09-B2] CollegeZone Mar 28, 2018
SoilChang pushed a commit to SoilChang/addressbook-level4 that referenced this pull request Apr 5, 2018
@deborahlow97 deborahlow97 changed the title [v1.3][T09-B2] CollegeZone [v1.5rc][T09-B2] CollegeZone Apr 12, 2018
@deborahlow97 deborahlow97 changed the title [v1.5rc][T09-B2] CollegeZone [v1.5][T09-B2] CollegeZone Apr 15, 2018
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