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

UI tab menu implementation & Refactoring #65

Merged
merged 8 commits into from Oct 10, 2019

Conversation

nordic96
Copy link

@nordic96 nordic96 commented Oct 9, 2019

  • implemented Typee layout (initial night theme mode) - refer to 7a7a405
  • refactored AddressBookParser to TypeeParser
  • added tabMenus.json for adding menus.
  • added JsonParser method readJsonFileIntoList to read array format.

for editing/adding menus in the tab panel, as of now, add a tab object in the tabMenus.json
ex)

[
  { "name" :  "Appointment" },
  { "name" :  "Typing Game" },
  { "name" :  "Calendar View" },
  { "name":  "Generate Report" }
]

@nordic96 nordic96 added the Type.Enhancement New feature or request label Oct 9, 2019
@nordic96 nordic96 added this to the v1.2 milestone Oct 9, 2019
@nordic96 nordic96 added the priority.High Must do label Oct 9, 2019
Copy link

@jun-ha0 jun-ha0 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

src/main/java/com/typee/model/Tab.java Show resolved Hide resolved
src/main/java/com/typee/storage/Storage.java Outdated Show resolved Hide resolved
src/main/java/com/typee/storage/TypeeStorage.java Outdated Show resolved Hide resolved
@brebeek
Copy link

brebeek commented Oct 9, 2019

LGTM

Copy link

@uggi121 uggi121 left a comment

Choose a reason for hiding this comment

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

LGTM! Can refactor/beautify a little bit after merge

Comment on lines 30 to 36
protected void updateItem(Tab tab, boolean empty) {
super.updateItem(tab, empty);
if (empty || tab == null || tab.getName() == null) {
setText(null);
} else {
setText(tab.getName());
}
Copy link

Choose a reason for hiding this comment

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

Looks good! Maybe rename boolean empty to isEmpty to emphasize that it's a boolean :) You can consider having another method isEmpty that checks the condition in line 32 to adhere to the SLAP principle

Copy link
Author

@nordic96 nordic96 Oct 9, 2019

Choose a reason for hiding this comment

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

Thanks for reminding me the SLAP again. Will update the parameter name to isEmpty instead. For the isEmpty() method, the super method force to set it empty and update the list cell object so I don't think it is necessary. Anyway, thanks for the review.

from Cell.java

     * @param item The new item for the cell.
     * @param empty whether or not this cell represents data from the list. If it
     *        is empty, then it does not represent any domain data, but is a cell
     *        being used to render an "empty" row.
     */
    protected void updateItem(T item, boolean empty) {
        setItem(item);
        setEmpty(empty);
        if (empty && isSelected()) {
            updateSelected(false);
        }
    }

@lyskevin lyskevin merged commit a7fc071 into AY1920S1-CS2103T-F14-3:master Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.High Must do Type.Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants