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

Jwen/show lyric #84

Merged
merged 12 commits into from
Mar 16, 2022
Merged

Jwen/show lyric #84

merged 12 commits into from
Mar 16, 2022

Conversation

jiabaow
Copy link
Collaborator

@jiabaow jiabaow commented Mar 15, 2022

Screen Shot 2022-03-15 at 11 52 36

Show lyrics of the request (track, artist) Remove activities that are already linked in the welcome screen from the greeting screen

@jiabaow jiabaow self-assigned this Mar 15, 2022
@jiabaow jiabaow marked this pull request as draft March 15, 2022 09:48
@jiabaow jiabaow marked this pull request as ready for review March 15, 2022 10:51
@jiabaow jiabaow linked an issue Mar 15, 2022 that may be closed by this pull request
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.

First reading looks good, just missing some documentation for non temporary code.
I will do a second read (and more in depth) later

@laurislopata
Copy link
Collaborator

LGTM

textViewLyric.text = response.body().lyrics
}
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think the logic of fetching data should be left in activity or moved somewhere else?

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 agree that way will be more consistent with the existing code we have, but from all the examples I have seen using retrofit they all put it like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was also fetching data. I did not use retrofit, I used okhttp3 and android.net.uri to build uri. I followed Maxime's logic in ItunesMusicApi.kt. He has created a class for fetching data with callbacks in it. So, when there was a click on the button, he just called this class to fetch data. I am not saying it is correct, but I liked that there is some separation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can keep the way you did!

@codeclimate
Copy link

codeclimate bot commented Mar 16, 2022

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

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

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

View more on Code Climate.

Copy link
Collaborator

@Tsathogguaa Tsathogguaa left a comment

Choose a reason for hiding this comment

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

Looks good to me. You have a few hardcoded strings that can be changed, as well as concatenating texts inside "setText" according to the CI warnings, but those can be fixed in future PRs along with other warnings we get. Good work!

@jiabaow
Copy link
Collaborator Author

jiabaow commented Mar 16, 2022

Looks good to me. You have a few hardcoded strings that can be changed, as well as concatenating texts inside "setText" according to the CI warnings, but those can be fixed in future PRs along with other warnings we get. Good work!

Thanks for the comments, I leave them hardcoded because they're temporary for showing the lyric, we need to change them later

@jiabaow jiabaow merged commit 9ad9749 into main Mar 16, 2022
@jiabaow jiabaow deleted the jwen/show-lyric branch March 16, 2022 13:01
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.

Find an API to get the lyric of a song and implement
5 participants