Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Jwen/set up scoreboard per playlist #203

Merged
merged 11 commits into from
Apr 26, 2022
Merged

Conversation

jiabaow
Copy link
Collaborator

@jiabaow jiabaow commented Apr 25, 2022

No description provided.

@jiabaow jiabaow self-assigned this Apr 25, 2022
@jiabaow jiabaow marked this pull request as ready for review April 25, 2022 19:36
@kamilababayeva kamilababayeva self-requested a review April 25, 2022 20:15
Copy link
Collaborator

@kamilababayeva kamilababayeva left a comment

Choose a reason for hiding this comment

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

I have left a small comment on a possible move of the function to usersRepo. Otherwise, PR looks good to me!

@@ -37,8 +60,8 @@ class ScoreBoardActivity : AppCompatActivity() {
}
}

private fun loadPlayers() {
dbRef.orderByChild("ranking")
private fun loadPlayersSortedBy(order: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

UsersRepo contains all the firebase calls connected with "users", should this function be moved there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you mean, however, It's more logical for me to put it here because this function is only used locally in scoreboard activity to load the data into the scoreboard.

Copy link
Owner

@MaximeZmt MaximeZmt left a comment

Choose a reason for hiding this comment

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

👍 Just some questions left in comments

Comment on lines 41 to 46
R.id.btsButton -> sortedBy = "ranking"
R.id.kpopButton -> sortedBy = "ranking"
R.id.imagDragonsButton -> sortedBy = "ranking"
R.id.billieEilishButton -> sortedBy = "ranking"
R.id.rockButton -> sortedBy = "ranking"
R.id.topTracksButton -> sortedBy = "ranking"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
R.id.btsButton -> sortedBy = "ranking"
R.id.kpopButton -> sortedBy = "ranking"
R.id.imagDragonsButton -> sortedBy = "ranking"
R.id.billieEilishButton -> sortedBy = "ranking"
R.id.rockButton -> sortedBy = "ranking"
R.id.topTracksButton -> sortedBy = "ranking"
R.id.btsButton -> sortedBy = "ranking"
R.id.kpopButton -> sortedBy = "ranking"
R.id.imagDragonsButton -> sortedBy = "ranking"
R.id.billieEilishButton -> sortedBy = "ranking"
R.id.rockButton -> sortedBy = "ranking"
R.id.topTracksButton -> sortedBy = "ranking"

Could store "ranking" into string.xml ? easier to modify after

Comment on lines +50 to +73
@Test
fun topBtnClick() {
onView(withId(R.id.topTracksButton)).perform(click())
}

@Test
fun kpopBtnClick() {
onView(withId(R.id.kpopButton)).perform(click())
}

@Test
fun billieEilishButtonClick() {
onView(withId(R.id.billieEilishButton)).perform(click())
}

@Test
fun imagineDragonsButtonClick() {
onView(withId(R.id.imagDragonsButton)).perform(click())
}

@Test
fun btsButtonClick() {
onView(withId(R.id.btsButton)).perform(click())
}
Copy link
Owner

Choose a reason for hiding this comment

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

Here it clicks, but does not miss a verification ? What should it test ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Normally, the verification of each should be the same as in rockBtnShouldSetUpRecycleView() and rockBtnShouldEnableRecycleViewClick(), but they are all testing the same lines of code, so to speed up CI and have the coverage rate, I just do click on each of them without the actual verification.

}

@Test
fun rockBtnShouldSetUpRecycleVie() {
Copy link
Owner

@MaximeZmt MaximeZmt Apr 26, 2022

Choose a reason for hiding this comment

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

Suggested change
fun rockBtnShouldSetUpRecycleVie() {
fun rockBtnShouldSetUpRecycleView() {

I think this is a typo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this typo still exist? I remember fixing it in commit 5b54240

Copy link
Owner

@MaximeZmt MaximeZmt left a comment

Choose a reason for hiding this comment

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

Just found a typo; otherwise all good 😀

Copy link
Owner

@MaximeZmt MaximeZmt left a comment

Choose a reason for hiding this comment

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

👍 nice work !

@codeclimate
Copy link

codeclimate bot commented Apr 26, 2022

Code Climate has analyzed commit 9db474a and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 87.4% (0.1% change).

View more on Code Climate.

@jiabaow jiabaow merged commit 7e85a84 into main Apr 26, 2022
@jiabaow jiabaow deleted the jwen/set-up-scoreboard-per-playlist branch April 26, 2022 07:55
@jiabaow jiabaow linked an issue Apr 27, 2022 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create a scoreboard (UI) for each playlist.(#199)
3 participants