-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update UG and Diagrams #143
Update UG and Diagrams #143
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor changes for consideration and a spelling mistake
docs/UserGuide.md
Outdated
|
||
Sorts the list of persons being viewed by name or date of last visit in ascending or descending order. | ||
|
||
Format: `sort paramater/order` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would make sense to write it as sort [n/ORDER] [d/ORDER]
? For consistency with the other command formats.
If you do decide this current format is better, take note of the "paramater" -> "parameter" spelling error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then that kind of assumes that you can just type sort
which I haven't built in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, fair enough!
docs/UserGuide.md
Outdated
Format: `sort paramater/order` | ||
|
||
* Sorts the displayed list of persons according to the specified order. | ||
* Order can be specified as ascending by leaving the order blank or **asc** **ascending** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a "/" between asc and ascending so it's clearer that they're both equal?
docs/UserGuide.md
Outdated
* Order can be specified as descending by **descending** or **desc** | ||
|
||
Examples: | ||
* `sort n/` sorts name in ascending order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps "sorts by name in ascending order."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
One thing to mention--when I try to view the Model Class Diagram on the website itself, it shows up without an arrowhead. But the UML diagram generated by the code has one, so I'm not sure if it's a MarkBind issue. To be checked: does the website display the UML diagram correctly?
Closes #136
Closes #137
Updated UG for Sort Command instructions.
Edited the Model Diagram to include PersonComparator dependency and specify remarks as 1 to clarify that we only allow one remark (as this is not abundantly obvious unlike the other terms.