MOB-5193 HP4U Data API #26
Conversation
sanjacurcic-taboola
requested review from
tomer-br-taboola and
voltfmanSlava
|
@sanjacurcic-taboola - When adding UI, please add screenshots of the relevant UI. |
app/build.gradle
Outdated
| applicationId "com.taboola.hp4udemoapplication" | ||
| minSdk 21 | ||
| targetSdk 32 | ||
| targetSdk 34 |
There was a problem hiding this comment.
@sanjacurcic-taboola - The latest SDK level is 36, so at the very least you can upgrade to it or to 35.
| implementation 'androidx.core:core-splashscreen:1.0.0-rc01' | ||
| implementation 'com.squareup.picasso:picasso:2.8' | ||
| //Taboola SDK | ||
| implementation 'com.taboola:android-sdk:3.8.13' |
There was a problem hiding this comment.
@sanjacurcic-taboola - Notice that you changed the major version (from three to four). I would make sure nothing was impacted.
There was a problem hiding this comment.
I've already checked the entire app, everything seems fine.
There was a problem hiding this comment.
@sanjacurcic-taboola - Remember you can only merge this once 4.0.17 has been released officially.
sanjacurcic-taboola
force-pushed
the
MOB-5193-hp4u-data-api
branch
from
8348835 to
2e01bc4
Compare
|
|
||
| override fun onViewCreated(view: View, savedInstanceState: Bundle?) { | ||
| super.onViewCreated(view, savedInstanceState) | ||
| if (homePage == null) createHomePage() |
There was a problem hiding this comment.
@sanjacurcic-taboola - Is there a reason where homePage wouldn't be null in this situation?
There was a problem hiding this comment.
When we click on some Article, a new screen will open. After returning from that screen to the HomePage screen, the onViewCreated will be called again and the HomePage fetchContent will be triggered again. To avoid this, I added this check. If the HomePage has already been initialized, it should not be initialized again.
There was a problem hiding this comment.
Why is the example project initializing homepage in a fragment? Wouldn't it be more relevant in an activity that holds fragments? Do you feel this simulates a publisher scenario?
| override fun onHomePageStatusChanged(active: Boolean) { | ||
| super.onHomePageStatusChanged(active) | ||
|
|
||
| if (active) { |
There was a problem hiding this comment.
@sanjacurcic-taboola - Maybe add a log/toast if home page isn't active?
| } | ||
|
|
||
| private fun swapItems(recommendationItems: HashMap<String, MutableList<TBLRecommendationHomePageDataApiItem>>) { | ||
| if (homePage == null) return |
There was a problem hiding this comment.
@sanjacurcic-taboola - I see there are many places where you are guarding the scenario where homePage is null. What could lead to this behavior? Also, do you want to log something for these scenarios?
There was a problem hiding this comment.
Removed if (homePage == null) return from here and responded in another comment.
|
|
||
| for (position in listWithSwappedItems.indices) { | ||
| val currentItem = listWithSwappedItems[position] | ||
| if (currentItem is Article) sectionName = currentItem.sectionName |
There was a problem hiding this comment.
@sanjacurcic-taboola - Could you refactor this into a when statement?
| if (currentItem is Article) sectionName = currentItem.sectionName | ||
| if (currentItem is Header) sectionStartPositionIndex = position + 1 | ||
|
|
||
| if (homePage!!.shouldSwapItemInSectionDataApi(sectionName, position, sectionStartPositionIndex)) { |
There was a problem hiding this comment.
@sanjacurcic-taboola - Force unwrapping is not something we should always do. Is there a way to re-write this?
| } | ||
| } | ||
|
|
||
| private fun getRecommendation( |
There was a problem hiding this comment.
@sanjacurcic-taboola - If a publisher goes over this code, do you believe they will understand what is going on here?
There was a problem hiding this comment.
Added comments that will help publisher understand the code
There was a problem hiding this comment.
@sanjacurcic-taboola - Thanks, that helps, but according to your comments, I believe the name of the method can be changed to provide better clarity.
tomer-br-taboola
left a comment
tomer-br-taboola
left a comment
There was a problem hiding this comment.
@sanjacurcic-taboola - Please go over the comments.
| } | ||
| DEFAULT_ARTICLE -> { | ||
| val viewHolder = HomePageItemViewHolder(view) | ||
| val viewHolder = if (isHomePageDataApiMode) HomePageDataApiItemViewHolder(view) else HomePageItemViewHolder(view) |
There was a problem hiding this comment.
val viewHolder = when(isHomePageApiMode) { true -> ... false -> ... }
| } | ||
| DEFAULT_ARTICLE -> { | ||
| val viewHolder = if (isHomePageDataApiMode) HomePageDataApiItemViewHolder(view) else HomePageItemViewHolder(view) | ||
| val viewHolder = when (isHomePageDataApiMode) { true -> HomePageDataApiItemViewHolder(view) false -> HomePageItemViewHolder(view) } |
There was a problem hiding this comment.
@sanjacurcic-taboola - Please apply formatting here.
|
|
||
| private val TAG = HomePageDataApiScreenFragment::class.java.simpleName | ||
| private var homePage: TBLHomePage? = null | ||
| private lateinit var homePage: TBLHomePage |
There was a problem hiding this comment.
@sanjacurcic-taboola - Since you changed it to lateinit, just make sure that any flow in your logic doesn't reference this variable before it is actually initialized. Because otherwise, there will be a crash.
tomer-br-taboola
left a comment
tomer-br-taboola
left a comment
There was a problem hiding this comment.
@sanjacurcic-taboola - I am approving, but please go over the last remaining comments.
Added new screen for HP4U Data API.