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

Create new UI and integrate all v1.2 changes #114

Merged
merged 11 commits into from
Oct 19, 2023

Conversation

keaganpzh
Copy link
Member

@keaganpzh keaganpzh commented Oct 18, 2023

Currently, the UI is the same as the original AddressBook. All changes done in iteration 1.2 have been individually tested, but have not been integrated with the UI. There are also redundant classes such as AddCommand which is deprecated due to the creation of more specific GuestAddCommand and VendorAddCommand classes.

Let's:

  • Create a new UI for WedLog
  • Integrate all new classes and ensure it works with the UI
  • Remove some redundant classes

Disclaimer: UI is not polished yet and looks abit fugly. I will improve it next PR.

Addresses #87.

@keaganpzh keaganpzh added the priority.High Must do label Oct 18, 2023
@keaganpzh keaganpzh requested a review from samuelim01 October 18, 2023 15:53
@keaganpzh keaganpzh marked this pull request as draft October 18, 2023 15:54
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Attention: 95 lines in your changes are missing coverage. Please review.

Comparison is base (198a7f8) 81.21% compared to head (32a20b1) 78.19%.
Report is 8 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #114      +/-   ##
============================================
- Coverage     81.21%   78.19%   -3.02%     
+ Complexity      651      649       -2     
============================================
  Files            99      101       +2     
  Lines          1900     1963      +63     
  Branches        210      214       +4     
============================================
- Hits           1543     1535       -8     
- Misses          318      386      +68     
- Partials         39       42       +3     
Files Coverage Δ
.../java/wedlog/address/commons/core/GuiSettings.java 76.92% <ø> (ø)
...wedlog/address/logic/parser/AddressBookParser.java 100.00% <ø> (ø)
...rc/main/java/wedlog/address/model/AddressBook.java 88.88% <100.00%> (+0.36%) ⬆️
src/main/java/wedlog/address/model/Model.java 100.00% <ø> (ø)
...c/main/java/wedlog/address/model/ModelManager.java 100.00% <100.00%> (ø)
...a/wedlog/address/model/person/UniqueGuestList.java 90.62% <100.00%> (+3.66%) ⬆️
...c/main/java/wedlog/address/logic/LogicManager.java 72.00% <33.33%> (-5.28%) ⬇️
src/main/java/wedlog/address/MainApp.java 0.00% <0.00%> (ø)
...c/main/java/wedlog/address/ui/StatisticsPanel.java 0.00% <0.00%> (ø)
src/main/java/wedlog/address/ui/VendorCard.java 0.00% <0.00%> (ø)
... and 5 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@keaganpzh keaganpzh requested review from tllshan and removed request for samuelim01 October 18, 2023 15:58
@keaganpzh
Copy link
Member Author

bruh how tf am i supposed to test the UI portions to satisfy codecov the original repo doesn't have it covered either

@keaganpzh keaganpzh marked this pull request as ready for review October 18, 2023 16:34
@samuelim01
Copy link

Could I clarify the difference between the RSVP status of YES, NO, and UNKNOWN? From my understanding, NO means the person is not coming for the wedding. The UI here seems to reflect that both NO and UNKNOWN means that they have not responded to the RSVP invite.

image

@keaganpzh
Copy link
Member Author

Could I clarify the difference between the RSVP status of YES, NO, and UNKNOWN? From my understanding, NO means the person is not coming for the wedding. The UI here seems to reflect that both NO and UNKNOWN means that they have not responded to the RSVP invite.

image

My intention behind designing the pie chart is that you can see the proportion of users who are confirmed to be coming at a glance. So I grouped those with RSVP status as no and unknown together to show that they are not coming (or unconfirmed).

We see what the others think?

@tllshan
Copy link

tllshan commented Oct 19, 2023

Could I clarify the difference between the RSVP status of YES, NO, and UNKNOWN? From my understanding, NO means the person is not coming for the wedding. The UI here seems to reflect that both NO and UNKNOWN means that they have not responded to the RSVP invite.
image

My intention behind designing the pie chart is that you can see the proportion of users who are confirmed to be coming at a glance. So I grouped those with RSVP status as no and unknown together to show that they are not coming (or unconfirmed).

We see what the others think?

I do think the current labels on the pie chart could be quite misleading. What do y'all think of having three sections to reflect yes, no and unknown?

Copy link

@samuelim01 samuelim01 left a comment

Choose a reason for hiding this comment

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

Thanks for your hard work! LGTM with a few nits. Ideally, I think this commit should be merged after #109 is fixed.

src/main/java/wedlog/address/ui/VendorListPanel.java Outdated Show resolved Hide resolved
src/main/java/wedlog/address/ui/VendorListPanel.java Outdated Show resolved Hide resolved
@tllshan tllshan linked an issue Oct 19, 2023 that may be closed by this pull request
@keaganpzh keaganpzh linked an issue Oct 19, 2023 that may be closed by this pull request
tllshan
tllshan previously approved these changes Oct 19, 2023
Copy link

@tllshan tllshan left a comment

Choose a reason for hiding this comment

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

LGTM!

* @return {@code RsvpStatistics}
*/
public RsvpStatistics getRsvpStatistics() {
return new RsvpStatistics(guests.getNumGuestsRsvpYes(), guests.getNumGuestsRsvpNo(),
Copy link

Choose a reason for hiding this comment

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

Perhaps it would be better to implement the logic of getting RSVP statistics within UniqueGuestList? would also eliminate the need for getters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I think the current implementation is fine, having our AddressBook class handle the creation of a RsvpStatistics object makes sense to me as the AddressBook has an overview of everything being tracked, and can derive RSVP information from its UniqueGuestList.

src/main/java/wedlog/address/ui/GuestCard.java Outdated Show resolved Hide resolved
@tllshan tllshan dismissed their stale review October 19, 2023 09:44

added comments

@samuelim01 samuelim01 mentioned this pull request Oct 19, 2023
10 tasks
@keaganpzh keaganpzh requested a review from tllshan October 19, 2023 12:34
Copy link

@tllshan tllshan left a comment

Choose a reason for hiding this comment

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

LGTM!

@tllshan tllshan merged commit a5db704 into AY2324S1-CS2103T-F11-2:master Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update GUI As a user with many guests, I want to view how many guests have RSVP'd
3 participants