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-T17-2] Traveez #70

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

Conversation

bjhoohaha
Copy link

No description provided.

Copy link

@EugeneTeu EugeneTeu left a comment

Choose a reason for hiding this comment

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

Interesting design as it looks v different from a normal MVC pattern

@@ -93,13 +107,14 @@ The `UI` component,

[[fig-LogicClassDiagram]]
.Structure of the Logic Component
image::LogicClassDiagram.png[]
image::BetterLogicClassDiagram.png[]

Choose a reason for hiding this comment

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

Can break up into smaller abstraction

@@ -115,21 +130,23 @@ NOTE: The lifeline for `DeleteCommandParser` should end at the destroy marker (X
=== Model component

.Structure of the Model Component
image::ModelClassDiagram.png[]
image::BetterModelClassDiagram.png[]

Choose a reason for hiding this comment

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

There should not be floating models in the diagrams. need an arrow to bind it to something


*Calendar Class Diagram:*+

image::CalendarClassDiagram.png[]

Choose a reason for hiding this comment

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

Calendar should not have an extra arrow to abstract class events.
Calendar can be placed below ReadOnlyCalendar to improve readability


The above diagram shows that ``Calendar`` stores information about the state of the user's calendar, including information about the user's current view as indicated by ``currentMonth``.

image::ShowSequenceDiagram.png[]

Choose a reason for hiding this comment

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

is the last Command Result box missing an arrow back to Logic manager?

exposes an unmodifiable ObservableList<Event> that can be 'observed' e.g. the UI can be bound to this list so that
the UI automatically updates when the data in the list change.

does not depend on any of the other three components, UI, logic and storage which are common through all the features

Choose a reason for hiding this comment

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

Point form?


=== Model Component

The implementation of the model class in TravEzy is to be a generic. Hence, the model object being instantiated

Choose a reason for hiding this comment

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

If I am not wrong, you guys are treating model as a data type here right?

pohlinwei and others added 25 commits November 10, 2019 17:19
* 'master' of https://github.com/AY1920S1-CS2103T-T17-2/main: (27 commits)
  Update UserGuide.adoc
  Update UserGuide.adoc
  Update UserGuide.adoc
  Update UserGuide.adoc
  Update UserGuide.adoc
  Update UserGuide.adoc
  Updates UG main
  checkstyle
  Update UserGuide.adoc
  Update UserGuide.adoc
  Updates UG
  Add files via upload
  Update UserGuide.adoc
  Add files via upload
  Add files via upload
  Delete check1.png
  Delete check2.png
  Update UserGuide.adoc
  Update UserGuide.adoc
  Update UserGuide.adoc
  ...

# Conflicts:
#	docs/UserGuide.adoc
Solve merge conflicts and update UG
Revert "Solve merge conflicts and update UG"
pohlinwei and others added 30 commits November 11, 2019 23:03
# Conflicts:
#	docs/DeveloperGuide.adoc
* 'master' of https://github.com/AY1920S1-CS2103T-T17-2/main:
  Fix developer guides and user guides
  Fix website
  Fix checkstyle
  Fix checkstyle
  Fix checkstyle
  Update Developer Guide
  Fix checkstyle
  v1.4 implementation
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.

6 participants