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

Margaux/local game screen #53

Merged
merged 23 commits into from
Mar 11, 2022
Merged

Margaux/local game screen #53

merged 23 commits into from
Mar 11, 2022

Conversation

zwierski
Copy link
Collaborator

Pull request for the gamescreen that works with a popup. The popup cannot be tested yet as it requires robolectric which I'm unable to make work.

buildPopup.setTitle("Solution")
buildPopup.setMessage("The song was " + song + " by " + artist)

buildPopup.setPositiveButton("Correct") { dialog, which ->
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

Copy link
Owner

Choose a reason for hiding this comment

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

not urgent now but should consider creating a method to send toast(...)

"Congrats!", Toast.LENGTH_SHORT).show()
}

buildPopup.setNegativeButton("Wrong") { dialog, which ->
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@zwierski zwierski marked this pull request as ready for review March 11, 2022 07:41
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.

Some changes will be needed in the future, but not urgent. Good for now 👍

}

// FIXME: Find a way to test the popup showing despite not being a component / make robolectric work
/*
Copy link
Owner

Choose a reason for hiding this comment

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

Can remove empty test in comment maybe ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's why she leaves the FIXME here

import androidx.appcompat.widget.AlertDialogLayout


class GamescreenActivity: AppCompatActivity() {
Copy link
Owner

Choose a reason for hiding this comment

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

maybe add javadoc for whole class ?

buildPopup.setTitle("Solution")
buildPopup.setMessage("The song was " + song + " by " + artist)

buildPopup.setPositiveButton("Correct") { dialog, which ->
Copy link
Owner

Choose a reason for hiding this comment

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

not urgent now but should consider creating a method to send toast(...)

Copy link
Collaborator

@jiabaow jiabaow left a comment

Choose a reason for hiding this comment

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

LGTM
The toast can be tested later in another PR as discussed

}

// FIXME: Find a way to test the popup showing despite not being a component / make robolectric work
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's why she leaves the FIXME here

android:id="@+id/toLocalGameButton"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:text="L O C A L G A M E"
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider using @string instead of hardcode it

@codeclimate
Copy link

codeclimate bot commented Mar 11, 2022

Code Climate has analyzed commit 86e9abc and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

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

This pull request will bring the total coverage in the repository to 94.2% (-1.7% change).

View more on Code Climate.

@MaximeZmt MaximeZmt merged commit 31d37fc into main Mar 11, 2022
@MaximeZmt MaximeZmt deleted the margaux/local_game_screen branch March 11, 2022 09:04
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.

3 participants