-
Notifications
You must be signed in to change notification settings - Fork 720
Conversation
@shamim-emon replace the red color with purple. Red means error. I'll review the code later today or tmrw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't review much in detail but overall LGTM! Left some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except what Iliyan mentioned.
@suyash01 thank you for reviewing! I'm planning to delegate reviewers to contributors so our project is even more community-driven |
1f1528b
to
bbcb263
Compare
@@ -332,6 +375,14 @@ class LoanViewModel @Inject constructor( | |||
} | |||
} | |||
|
|||
private fun loadCompletedLoans() { | |||
completedLoans = allLoans.filter { loan -> loan.percentPaid == 1.0 }.toImmutableList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shamim-emon could you replace == with >= to patch #3546?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nvllz sure
Pull request (PR) checklist
Please check if your pull request fulfills the following requirements:
Screen recording of your changes (if applicable):
video.mp4
What's changed?
Describe with a few bullets what's new:
Risk factors
What may go wrong if we merge your PR?
In what cases won't your code work?
Does this PR close any GitHub issues? (do not delete)
Troubleshooting GitHub Actions (CI) failures ❌
Pull request checks failing? Read our CI Troubleshooting guide.