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

Replace Phone with Priority #50

Conversation

jonahtanjz
Copy link
Collaborator

@jonahtanjz jonahtanjz commented Sep 29, 2020

  • Replace all instances of Phone with Priority
  • Priority is optional in Add command, default to LOW if not specified
  • Add colours to different priorities in UI
  • Update JUnit test to work with Priority

@jonahtanjz jonahtanjz added this to the v1.2 milestone Sep 29, 2020
@jonahtanjz jonahtanjz self-assigned this Sep 29, 2020
@jonahtanjz jonahtanjz linked an issue Sep 29, 2020 that may be closed by this pull request
@rolandyuwy rolandyuwy self-requested a review September 30, 2020 09:18
@bchenghi bchenghi self-requested a review October 3, 2020 08:25
@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2020

Codecov Report

Merging #50 into master will decrease coverage by 0.48%.
The diff coverage is 76.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #50      +/-   ##
============================================
- Coverage     72.31%   71.82%   -0.49%     
- Complexity      400      405       +5     
============================================
  Files            70       70              
  Lines          1228     1253      +25     
  Branches        124      126       +2     
============================================
+ Hits            888      900      +12     
- Misses          308      320      +12     
- Partials         32       33       +1     
Impacted Files Coverage Δ Complexity Δ
.../java/seedu/address/logic/commands/AddCommand.java 100.00% <ø> (ø) 8.00 <0.00> (ø)
.../java/seedu/address/model/util/SampleDataUtil.java 20.00% <ø> (ø) 1.00 <0.00> (ø)
src/main/java/seedu/address/ui/PersonCard.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...main/java/seedu/address/model/person/Priority.java 82.60% <82.60%> (ø) 11.00 <11.00> (?)
...c/main/java/seedu/address/model/person/Person.java 97.56% <88.88%> (ø) 21.00 <1.00> (ø)
...java/seedu/address/logic/commands/EditCommand.java 97.01% <100.00%> (ø) 12.00 <0.00> (ø)
...a/seedu/address/logic/parser/AddCommandParser.java 100.00% <100.00%> (ø) 5.00 <0.00> (ø)
...ain/java/seedu/address/logic/parser/CliSyntax.java 83.33% <100.00%> (ø) 1.00 <0.00> (ø)
.../seedu/address/logic/parser/EditCommandParser.java 92.30% <100.00%> (ø) 11.00 <0.00> (ø)
...in/java/seedu/address/logic/parser/ParserUtil.java 97.22% <100.00%> (ø) 14.00 <2.00> (ø)
... and 2 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 f1d1c27...db983c1. Read the comment docs.

Copy link
Collaborator

@bchenghi bchenghi left a comment

Choose a reason for hiding this comment

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

The changes look great! I added some comments below.

src/main/java/seedu/address/ui/PersonCard.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/ui/PersonCard.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@rolandyuwy rolandyuwy left a comment

Choose a reason for hiding this comment

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

Just a few small suggestions.

src/main/java/seedu/address/ui/PersonCard.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/model/person/Priority.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/model/person/Priority.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/model/person/Priority.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/ui/PersonCard.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@bchenghi bchenghi left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@rolandyuwy
Copy link
Collaborator

LGTM

@songyiang songyiang merged commit 1a99fd0 into AY2021S1-CS2103T-F13-4:master Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants