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

Add ability to filter meeting list based on specific contact / whether to display all meetings #153

Merged
merged 16 commits into from
Oct 27, 2020

Conversation

sebastiantoh
Copy link

@sebastiantoh sebastiantoh commented Oct 25, 2020

Closes #10

Changes:

  • Added meeting list [c/CONTACT_INDEX] [a/]
    • By default, only upcoming meetings are displayed (aka meeting list)
  • Updated UI so that past meetings have lower opacity

Meeting list

@sebastiantoh sebastiantoh added this to the v1.3 milestone Oct 25, 2020
@codecov-io
Copy link

codecov-io commented Oct 25, 2020

Codecov Report

Merging #153 into master will increase coverage by 0.05%.
The diff coverage is 79.51%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #153      +/-   ##
============================================
+ Coverage     74.72%   74.78%   +0.05%     
- Complexity     1201     1222      +21     
============================================
  Files           166      167       +1     
  Lines          3846     3922      +76     
  Branches        537      556      +19     
============================================
+ Hits           2874     2933      +59     
- Misses          784      798      +14     
- Partials        188      191       +3     
Impacted Files Coverage Δ Complexity Δ
...edu/address/logic/commands/meeting/AddCommand.java 76.47% <ø> (ø) 11.00 <0.00> (ø)
.../address/logic/commands/meeting/DeleteCommand.java 100.00% <ø> (ø) 7.00 <0.00> (ø)
...address/logic/commands/reminder/DeleteCommand.java 100.00% <ø> (ø) 7.00 <0.00> (ø)
...u/address/logic/commands/reminder/ListCommand.java 100.00% <ø> (ø) 3.00 <0.00> (ø)
...ss/logic/parser/meeting/MeetingCommandsParser.java 44.44% <0.00%> (-11.12%) 4.00 <0.00> (-1.00)
src/main/java/seedu/address/ui/MeetingCard.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ddress/logic/parser/meeting/ListCommandParser.java 76.92% <76.92%> (ø) 5.00 <5.00> (?)
...rc/main/java/seedu/address/model/ModelManager.java 88.59% <87.50%> (-0.30%) 62.00 <1.00> (+1.00) ⬇️
...du/address/logic/commands/meeting/ListCommand.java 88.23% <88.00%> (-11.77%) 14.00 <8.00> (+11.00) ⬇️
...ain/java/seedu/address/logic/parser/CliSyntax.java 95.65% <100.00%> (+0.19%) 1.00 <0.00> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d8e52a...76dd863. Read the comment docs.

@sebastiantoh sebastiantoh marked this pull request as ready for review October 25, 2020 14:35
Copy link

@hakujitsu hakujitsu left a comment

Choose a reason for hiding this comment

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

When testing, something interesting happened: I created an upcoming meeting and used meeting list to display all meetings. After the meeting was over, I retyped meeting list, and the meeting was greyed out, but still present. I think there's a need to refresh the contents of the list in this case?

Other than this issue, the code looks good! Great decision to use ListMeetingDescriptor

@sebastiantoh
Copy link
Author

When testing, something interesting happened: I created an upcoming meeting and used meeting list to display all meetings. After the meeting was over, I retyped meeting list, and the meeting was greyed out, but still present. I think there's a need to refresh the contents of the list in this case?

Other than this issue, the code looks good! Great decision to use ListMeetingDescriptor

Great catch, fixed! Thank you!

@hakujitsu
Copy link

Hmm, I pulled again to test but the meeting still remains greyed out for me and doesn't disappear, could you check again?

@sebastiantoh
Copy link
Author

That's weird.

Just to confirm, the steps to reproduce are as follows: (assuming current time is 26 Oct 2020, 18:32)

  1. Open up app
  2. Execute meeting add c/1 m/fnakldsnlk d/2020-10-26 18:32 du/1
  3. Execute meeting list
  4. Wait for 18:34
  5. Execute meeting list
    • Newly created meeting should disappear, but appears in the list (although greyed out)?

@hakujitsu
Copy link

My bad, I tried again and it seems to be fixed. Will merge!

@hakujitsu hakujitsu merged commit 7d6aa47 into AY2021S1-CS2103T-T11-1:master Oct 27, 2020
@sebastiantoh sebastiantoh deleted the meeting-filters branch October 27, 2020 07:47
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.

As a well-connected salesman, I can see a history of the number of contacts made with someone
3 participants