New details screen using Jetpack compose#50
New details screen using Jetpack compose#50rohithThammaiah wants to merge 19 commits intoAkshayChordiya:masterfrom
Conversation
# Conflicts: # app/build.gradle
rohithThammaiah
changed the title
AkshayChordiya
left a comment
AkshayChordiya
left a comment
There was a problem hiding this comment.
Thanks for your contribution. It's really amazing work 🏅
I left a few comments to improve upon 😄
|
|
||
| // Coil | ||
| implementation "io.coil-kt:coil:$coilVersion" | ||
| implementation "dev.chrisbanes.accompanist:accompanist-coil:0.3.1" |
There was a problem hiding this comment.
It's best to keep the version as a constant in project/build.gradle
|
|
||
| /** | ||
| * Gets the particular article from database for | ||
| * the give articleId |
There was a problem hiding this comment.
Minor typo
| * the give articleId | |
| * the given [articleId] |
There was a problem hiding this comment.
You can skip the @param code comment
| // 1. Start with loading | ||
| emit(ViewState.loading()) | ||
|
|
||
| // 2. Fetch and update state particular article from database |
There was a problem hiding this comment.
| // 2. Fetch and update state particular article from database | |
| // 2. Fetch the news article |
| /** | ||
| * Get News article for given articleId | ||
| */ | ||
| @Query("SELECT * FROM news_article WHERE id = :articleId") |
There was a problem hiding this comment.
Minor suggestion to use constant
| @Query("SELECT * FROM news_article WHERE id = :articleId") | |
| @Query("SELECT * FROM ${NewsArticles.tableName} WHERE id = :articleId") |
| } | ||
|
|
||
| /** | ||
| * Get News article for given articleId |
There was a problem hiding this comment.
| * Get News article for given articleId | |
| * Get news article for the specified [articleId] |
| Column(modifier = Modifier.fillMaxWidth().fillMaxHeight(), | ||
| verticalArrangement = Arrangement.Center, | ||
| horizontalAlignment = Alignment.CenterHorizontally) { |
There was a problem hiding this comment.
Minor suggestion to put all parameters on new line for cleaner look
| Column(modifier = Modifier.fillMaxWidth().fillMaxHeight(), | |
| verticalArrangement = Arrangement.Center, | |
| horizontalAlignment = Alignment.CenterHorizontally) { | |
| Column( | |
| modifier = Modifier.fillMaxWidth().fillMaxHeight(), | |
| verticalArrangement = Arrangement.Center, | |
| horizontalAlignment = Alignment.CenterHorizontally | |
| ) { |
| Column(modifier = Modifier.fillMaxWidth().fillMaxHeight(), | ||
| verticalArrangement = Arrangement.Center, | ||
| horizontalAlignment = Alignment.CenterHorizontally) { |
There was a problem hiding this comment.
Same suggestion as above to put all parameters on new line
| <item name="colorAccent">@color/colorAccent</item> | ||
| </style> | ||
|
|
||
| <!-- Base application theme. --> |
There was a problem hiding this comment.
Could we use the base AppTheme instead of creating this new one 🤔
There was a problem hiding this comment.
In the NewsDetailsActivity the ActionBar is set using compose, so I wanted to hide the default ActionBar which the activity provides from the theme. Can we can use the same theme and hide the ActionBar from the theme in onCreate using supportActionBar?.hide()?
There was a problem hiding this comment.
Ah I see. Got it now, in that case it's best to keep to have new theme like you did and perhaps update the code comment above it saying "Base theme for Compose"
AkshayChordiya
left a comment
AkshayChordiya
left a comment
There was a problem hiding this comment.
Great work, thank you so much 🙌
|
@rohithThammaiah Can you resolve the merge conflicts and make the PR ready for review? So I can review it and merge it |
Master to Fork
# Conflicts: # app/build.gradle # app/src/main/java/com/akshay/newsapp/news/ui/activity/NewsActivity.kt
2 participants
Fetching details from the database (Not sure if it's the right way). Had to migrate to ViewBinding because of some issue with synthetics in Kotlin version
1.4.10.#47