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

Branch ui #109

Merged
merged 6 commits into from Oct 19, 2019
Merged

Branch ui #109

merged 6 commits into from Oct 19, 2019

Conversation

chowyiyin
Copy link

Add right panel
Add expand person command and expand policy command

}

Person personToExpand = lastShownList.get(index.getZeroBased());
model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS);

Choose a reason for hiding this comment

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

I think this line should be removed since it negates search result. For instance, if I first search for all people with the surname 'Tan' with findperson tan, it gives me a list of everyone with the surname 'Tan'. Then if I choose to expand a person with expandperson 1, it expands the person, but the filtered list goes back to showing everyone in the address book. If the person is trying to review each person in a particular find query then it could be inconvenient for him. I'm not sure haha what do the rest of you guys think?

Choose a reason for hiding this comment

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

Yup i agree

}

Policy policyToExpand = lastShownList.get(index.getZeroBased());
model.updateFilteredPolicyList(PREDICATE_SHOW_ALL_POLICIES);

Choose a reason for hiding this comment

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

Same as above.

@olivercheok20
Copy link

Hey Yiyin! This is super super impressive haha great work with the JavaFX. Just one comment though, when I try to update information about a person that's already expanded, or any of the policies that he possesses, the expanded card doesn't update. The same thing with the expanded policies. Thanks :)

@larrylawl larrylawl merged commit 8ac341c into AY1920S1-CS2103-F09-4:master Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants