New details screen using Jetpack compose #50
Conversation
# Conflicts: # app/build.gradle
rohithThammaiah
changed the title
app/build.gradle
Outdated
|
|
||
| // 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
app/src/main/res/values/styles.xml
Outdated
| <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"
|
@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