-
Notifications
You must be signed in to change notification settings - Fork 54
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
[W17-1] The Food Diary #38
base: master
Are you sure you want to change the base?
[W17-1] The Food Diary #38
Conversation
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.
Hi team, some comments:
- Do standardise the name of your application.
- Do fill in the responsibilities of each member in the 'About Us' page as well.
- Remember to update the comments as well when reusing/adapting existing code.
- Do make the stated update to your README.adoc with regards to the build status badge(s).
Comments on your User and Developer Guides will be provided separately (likely as annotated PDFs on Luminus).
Looks like the groundwork for the project has been laid (renaming AB to FD, adding in some commands, classes and updating the parsers). Do keep up the work and remember to update your guides/UI image as you go along. 👍
docs/AboutUs.adoc
Outdated
_{The dummy content given below serves as a placeholder to be used by future forks of the project.}_ + | ||
{empty} + | ||
We are a team based in the http://www.comp.nus.edu.sg[School of Computing, National University of Singapore]. | ||
EatVago is a desktop food diary application. This is adapted and modified from Address Book Level 4 |
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 try to standardise the name of the application across the docs.
docs/AboutUs.adoc
Outdated
Role: Team Lead + | ||
Responsibilities: UI | ||
Role: Developer, Team Carry + | ||
Responsibilities: |
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 fill in the responsibilities assigned to each member as well.
|
||
public static final String COMMAND_WORD = "addName"; | ||
|
||
public static final String MESSAGE_USAGE = COMMAND_WORD + ": Adds a names to your profile " |
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.
name* might be more appropriate here
+ "diary.\n" | ||
+ "Parameters: " | ||
+ "INDEX (Must be a positive integer) " | ||
+ PREFIX_REVIEWENTRY + "REVIEW " |
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.
Could be more consistent with the case here
import seedu.address.logic.parser.exceptions.ParseException; | ||
|
||
/** | ||
* Parses input arguments and creates a new AddCommand object |
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.
Is an AddName
or AddNameCommand
object the intended creation here?
* Creates a new Restaurant with the newly added specified {@code Review} | ||
*/ | ||
private static Restaurant createRestaurantWithNewReview(Restaurant restaurantReviewed, Review reviewToAdd) { | ||
assert restaurantReviewed != null; |
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.
Good that you made use of an assert here
case AddReviewCommand.COMMAND_WORD: | ||
return new AddReviewCommandParser().parse(arguments); | ||
|
||
|
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.
Take note of the extra line here
import seedu.address.model.restaurant.UniqueRestaurantList; | ||
|
||
/** | ||
* Wraps all data at the address-book level |
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.
Remember to update this comment
public void setAddressBookFilePath(Path addressBookFilePath) { | ||
requireNonNull(addressBookFilePath); | ||
this.addressBookFilePath = addressBookFilePath; | ||
public void setName(String name) { |
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 a gentle reminder that the application should be for a single user
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.
This is just to allow the single user to change his name as he likes.
README.adoc
Outdated
https://ci.appveyor.com/project/damithc/addressbook-level4[image:https://ci.appveyor.com/api/projects/status/3boko2x2vr5cc3w2?svg=true[Build status]] | ||
https://coveralls.io/github/se-edu/addressbook-level4?branch=master[image:https://coveralls.io/repos/github/se-edu/addressbook-level4/badge.svg?branch=master[Coverage Status]] | ||
https://www.codacy.com/app/damith/addressbook-level4?utm_source=github.com&utm_medium=referral&utm_content=se-edu/addressbook-level4&utm_campaign=Badge_Grade[image:https://api.codacy.com/project/badge/Grade/fc0b7775cf7f4fdeaf08776f3d8e364a[Codacy Badge]] | ||
https://travis-ci.org/cs2103-ay1819s2-w17-1/main[image:https://travis-ci.org/cs2103-ay1819s2-w17-1/main.svg?branch=master["Build Status", link="https://travis-ci.org/cs2103-ay1819s2-w17-1/main"]] |
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.
Please update your badge(s) accordingly to ensure they point to the build status of your repo. Issue here is that your 'cs2103-ay1819s2-w17-1' has to be in uppercase.
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.
Hi team, some comments:
- Good that you have updated the implementation details of your features in the Developer Guide. However, in addition to how the feature is implemented, do include details on why it is implemented that way, and what other alternatives you considered in the process.
- You could try to make use of diagrams learnt in tutorials, code snippets etc. to elaborate on your implementation in the Developer Guide as well.
- For the webpage feature, do carefully consider graceful handling of exceptions such as an unreachable weblink. You could brainstorm for ideas on managing such an event by going back to your intended target audience and value proposition.
- Good that the User Guide has been updated with some features yet to come in future releases as well. Do remember to update them for all features as you go along.
- Do furnish your User Guide with further details of your features (E.g. are duplicates allowed?).
- Do note the recommended order of variables declared within your classes, based on the style guide (Depending on visibility).
- Do try to be more consistent about the naming of your commits (E.g. all to make use of the imperative mood?).
- For the upcoming milestone, do ensure your code is RepoSense compatible.
private static final String DOMAIN_FIRST_CHARACTER_REGEX = "[^\\W_]"; // alphanumeric characters except underscore | ||
private static final String DOMAIN_MIDDLE_REGEX = "[a-zA-Z0-9.-]*"; // alphanumeric, period and hyphen | ||
private static final String DOMAIN_LAST_CHARACTER_REGEX = "[^\\W_]$"; | ||
public static final String VALIDATION_REGEX = "^(http:\\/\\/|https:\\/\\/)" + LOCAL_PART_REGEX + "." |
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 take note of the order for variables in the class, as suggested in the style guide.
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.
Hi @sharan8, what if the public variable requires the private variables above it?
/** | ||
* Constructs an {@code Weblink}. | ||
* | ||
* @param weblink A valid email address. |
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.
Are you expecting an email address or a web link here?
@@ -149,19 +149,19 @@ image::LogicClassDiagram.png[width="800"] | |||
The _Sequence Diagram_ below shows how the components interact with each other for the scenario where the user issues the command `delete 1`. | |||
|
|||
.Component interactions for `delete 1` command | |||
image::SDforDeletePerson.png[width="800"] | |||
image::SDforDeleteRestaurant.png[width="800"] |
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 update the sequence diagrams for your morphed product where relevant
@@ -121,7 +121,7 @@ | |||
} | |||
|
|||
.cell_big_label { | |||
-fx-font-family: "Segoe UI Semibold"; | |||
-fx-font-family: "Segoe UI"; |
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.
Hi @weixin-koh, just wanted to check which of these fields you're having problems manipulating. Changing the font family for .cell_big_label
appears to work on my Mac. You might want to zero in on the font issue by checking if changing the font-size works as well.
Here's an external reference that I found with regards to integrating custom fonts - http://www.guigarage.com/2014/10/integrate-custom-fonts-javafx-application-using-css/
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.
Hi @sharan8, the one that I was having problems manipulating is actually the CSS font-family field for the HTML files, more specifically for .cell_placeholder_web
as the font would appear to be Times New Roman when I specified it as "Segoe UI Semibold". However, I followed the external reference you gave and it works now! Thank you :)
Just curious though, it seems that the CSS for HTML files require an import of the font, but how is JavaFX able to access all my local fonts without importing? (I changed the -fx-font-family
in .cell_big_label
to a custom font I downloaded and it worked without having to import the font in the CSS file)
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.
Great that it works! @weixin-koh
I'm not entirely sure how it may be able to access or interpret local fonts, and it could be a font that's present in JavaFX itself as well (you can check this by running javafx.scene.text.Font.getFamilies();
). Regardless, do confirm if the font works as expected on your team members' workstations too. I'm guessing embedding the font files directly under resources would be best.
Using web fonts is another alternative that introduces a dependency, but works - http://fxexperience.com/2012/12/use-webgoogle-fonts-in-javafx/
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.
Yup I added the font file into the resources folder! I ran the line of code and it showed all of the fonts in my computer, so I think JavaFX just has a way to read in all your local fonts? Thanks so much for your help @sharan8!
…tReviewUtil which stores the constant strings for the default review entries
…cks for the command parser
…1 and 5 inclusive.
…into sortByRatings * 'master' of https://github.com/cs2103-ay1819s2-w17-1/main: fix typo in note callout shift user and dev guide logo down to welcome section add logos to intro page and PPP updated docs and sample data reviews commented qz tests ug, dg ppp of skpai27 edits removed phone and email to check if restaurant is equal since items are removed from the card if absent edited aboutUS page updated tests updated visitWeb error messages updated dg on manual testing updated weblink validation check added sample data w/o reviews # Conflicts: # docs/UserGuide.adoc
fix display error
…into sortByRatings * 'master' of https://github.com/cs2103-ay1819s2-w17-1/main: fix display error # Conflicts: # docs/UserGuide.adoc
Update docs
Edited documentation for ug, ppp of skpai27
…into sortByRatings * 'master' of https://github.com/cs2103-ay1819s2-w17-1/main: documentation ug, ppp changes updated UG added image to ppp update docs updated docs # Conflicts: # docs/UserGuide.adoc
updated test case and docs
…into sortByRatings * 'master' of https://github.com/cs2103-ay1819s2-w17-1/main: final updated doucments Update UserGuide.adoc updated docs updated grammar updated documents updated test case and docs # Conflicts: # docs/UserGuide.adoc
Minor edits to UG, DG and PPP
No description provided.