-
Notifications
You must be signed in to change notification settings - Fork 0
HomeViewModel/ScoreRepositoryRefactor #27
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
Conversation
Commented them out since they currently aren't being used
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.
Amy, this is really great work, genuinely! I know I left a lot of comments, but it's so awesome to see that you have an understanding of MVVM. We're relatively close to this being ready, just address my comments and then you should be good.
app/src/main/java/com/cornellappdev/score/model/ScoreRepository.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/model/ScoreRepository.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/model/ScoreRepository.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/model/ScoreRepository.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/viewmodel/HomeViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/viewmodel/HomeViewModel.kt
Outdated
Show resolved
Hide resolved
zachseidner1
left a comment
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.
Oops my old review told you to refactor all the filters to use flows, even though that's the way it should be done, I realized it's probably not worth it for us right now. I want you to not have to go through an entire refactor so let's just leave a TODO comment for now. The current solution works and is more concise.
…ndroid into Amy/games-model
|
I think the formatColor function needs to be kept since it parses the String into a valid Color input and applies the appropriate alpha, so I feel like it's a little long to put in one line and I also feel like it needs documentation? I just moved the function into the ScoreRepository which seems a bit off so let me know if you see a better way to approach this. |
zachseidner1
left a comment
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.
Nice job with this refactor! We're almost there. I appreciate your work 🙏
app/src/main/java/com/cornellappdev/score/components/SportCard.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/model/ScoreRepository.kt
Outdated
Show resolved
Hide resolved
zachseidner1
left a comment
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.
Amazing work! 🥳
Overview
Addressed issue #21
Screenshots (delete if not applicable)
UI looks the same