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

[CS2103T-T11-4] TravelPal #53

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

Conversation

teoha
Copy link

@teoha teoha commented Sep 18, 2019


|`* * *` |user |add a new person |
[appendix]
== Use Cases

Choose a reason for hiding this comment

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

For use cases, the System, Actor and Preconditions are not clearly defined. Good to have that defined clearly before each use case to avoid ambiguity


*Product Name*
[appendix]
== Glossary

Choose a reason for hiding this comment

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

Would be good to have 'Trip Manager' in your Glossary, slightly unclear as to what it is


Steps 5a1-5a2 are repeated until no clashes occur between trips

<span class="underline">Use case: UC2 – Edit Trip</span>

Choose a reason for hiding this comment

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

Formatting error!

=== Use case: Delete person
6. TravelPal shows the list of **Trips**

Use case ends.

Choose a reason for hiding this comment

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

The "Use case ends." formatting is different for each use case, should be consistent

image::us1.PNG[]
image::us2.PNG[]
image::us3.PNG[]
image::us4.PNG[]
Copy link

Choose a reason for hiding this comment

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

User stories generally in correct format, and benefits do match the functions. You might want to re-look the last few user stories, particularly the ones in `us4.PNG'.

  • Sight-seeing enthusiast - maybe make it universal and not just in Singapore?
  • Travel/tour guide, teacher - can combine into single user story (since both are planning trips for others)?
  • Salesperson - feels like the user story not relevant to the user.
  • Accountant - I think "decide if the expenses are appropriate" isn't very well-defined.

You might want to add the benefits for more user stories too, I think it would help to flesh them out!

Otherwise, the other images are generally okay! Keep up the good work 👍👍👍

== Product Survey
6. Application does not depend on online resources to operate

7. Products is not required to make decisions for the user
Copy link

Choose a reason for hiding this comment

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

Might want to make this point clearer? Not too sure what do the products refer to. Also, there's a minor grammar error to be changed!


[[private-contact-detail]] Private contact detail::
A contact detail that is not meant to be shared with others
5. A novice user should be able to navigate without prior experience
Copy link

Choose a reason for hiding this comment

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

Are these really NFR? Seems like part of a user story!

. Should work on any <<mainstream-os,mainstream OS>> as long as it has Java `11` or above 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.
1. Should work on any [mainstream OS] as
Copy link

Choose a reason for hiding this comment

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

Formatting for "mainstream OS"


* ...
* ...
**Mainstream OS** - Windows, Linux, Unix, OS-X
Copy link

Choose a reason for hiding this comment

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

Do take note that this glossary is part of the developer's guide. Are these terms really necessary?

Comment on lines +606 to +609
2. 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.
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 make this a bit more concrete so that it's easier for developers to test!

teoha and others added 24 commits October 27, 2019 23:47
- Wrong alignment for DiaryLine photoCardsDisplay when graphic is to be placed on the left
- Refactored magic number for padding in DiaryLine to class level constant
- add complete introductory sections to diary section
- add command sections (without intended images currently) to diary section
- add tags to user and developer guide diary sections for display in project portfolio
- add initial draft for project portfolio page
- change link in AboutUs to link to project portfolio page
- fix ImageChooser import path conflict since moving from parser.diary.fileutil to commons.util
- readd overloaded constructor for CommandResult to maintain original CommandResult constructor use
- fix minor merge conflict in PageStatus

# Conflicts:
#	src/main/java/seedu/address/model/appstatus/PageStatus.java
Documentation and more tests
ang-zeyu and others added 30 commits November 11, 2019 23:35
Short ug for exit
Updating UG (I had put the description of the Event Inventory List in…
…S, but I had forgot to upload it on the GitHub Team Repo)
Uploading my PPP (I had already uploaded it on Monday night on LumiNU…
(already done on Tuesday, submitted on LumiNUS as well)
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.

None yet

9 participants