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

margaux/artwork-on-answer #256

Merged
merged 4 commits into from
May 6, 2022
Merged

margaux/artwork-on-answer #256

merged 4 commits into from
May 6, 2022

Conversation

zwierski
Copy link
Collaborator

@zwierski zwierski commented May 6, 2022

This PR adds, in the local buzzer game, the feature of the guessed song's album artwork being displayed on the answer card alongside the title.
Other minor adjustments were made, such as the artist's name appearing next to the title, and some UI adjustments on the size + color of the answer card.
pr_artwork1

@codeclimate
Copy link

codeclimate bot commented May 6, 2022

Code Climate has analyzed commit 2b4c6b8 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 82.2% (0.0% change).

View more on Code Climate.

@zwierski zwierski linked an issue May 6, 2022 that may be closed by this pull request
@zwierski zwierski marked this pull request as ready for review May 6, 2022 14:26
@MaximeZmt MaximeZmt self-requested a review May 6, 2022 14:33
@jiabaow jiabaow self-requested a review May 6, 2022 14:41
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

class BuzzerScreenActivity : GameActivity() {

private val MAX_N_PLAYERS = 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have no more limitations on the maximum number of players?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the limitation to 4 players is still there, but this val was only used in a check before I figured out how to directly fetch the player names from the intent, so it's not useful anymore.

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 ! 1 minor changes requested, otherwise all good 😃

findViewById<TextView>(R.id.answerText).text=title
val artist = gameManager.getCurrentSong().getArtistName()
findViewById<TextView>(R.id.songTitle).text= "$title - $artist"
Glide.with(ctx).load(gameManager.getCurrentSong().getArtworkUrl()).override(artworkDim, artworkDim).into(findViewById(R.id.songArtwork))
Copy link
Owner

Choose a reason for hiding this comment

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

Oh I have never heard of that Library. After having a look on internet, it may be great to use it everywhere. I will check later in my refactor if it is worth it or not.

android:layout_marginStart="30dp"
android:layout_marginEnd="30dp"
android:background="#AAAAAA"
android:background="#DDDDDD"
Copy link
Owner

Choose a reason for hiding this comment

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

You should use Colors.xml

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 !

@zwierski zwierski merged commit 49b9a08 into main May 6, 2022
@zwierski zwierski deleted the margaux/artwork-on-answer branch May 6, 2022 16:21
@zwierski zwierski self-assigned this May 8, 2022
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.

Add the artwork to the answer card in the buzzer game
3 participants