-
Notifications
You must be signed in to change notification settings - Fork 1
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
Re-Adding Friend Page features after reset #43
Conversation
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.
This PR looks good, I would consider modifying just the graphics but good job on the code! Dividing model and view was the right decision. But, before we can approve this the code must build on CI and must be tested. PR set aside while waiting for tests.
- Dynamic toasts based on which button is clicked
Still tests missing, add them and we will be fine. Again really a great job 👍 keep up the good work 💯 ! |
This design looks good to me 👍 Don't forget to check to update with the main branch before your development |
- Tests Incomplete
- Able to pass info from friends page to info page - able to navigate from infopage > game page - wrote tests, all passed when checked individually
This reverts commit 8f9c6bb.
- Same issue as before
- added ID for game fragment
Wrote tests, have yet to fix UI
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 !
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 as well :) 👍
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.
In general look really good. The UI could need some adjustments to look like the rest of the app. The text in InfoPageFragment is barely readable. Good job on the test coverage!
data class FriendsInfo( | ||
val username : String, | ||
val gamesPlayed : Int, | ||
val gamesWon : Int, |
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.
It seems a bit weird to me to have this class here. But I am not sure. Maybe this could be in models??
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.
You're right, let me move that.
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.
I will also work on changing to UI in the next sprint to match the rest of the application. Didn't have enough time for that this week
val friendName : TextView = itemView.findViewById(R.id.friendName) | ||
val winRate : TextView = itemView.findViewById(R.id.winRateText) | ||
val gamesPlayed : TextView = itemView.findViewById(R.id.gamesPlayedText) | ||
val gamesWon : TextView = itemView.findViewById(R.id.gamesWonText) |
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.
I think its best practice to use bindings instead of findViewByID. It reduces the boilerplate code
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.
I will discuss this with your tomorrow, and I will fix it before the next PR! Thank you for the comments :)
Added navigation, new info page and wrote tests.