feat: Changed channel content from chips to swipable tabs #6026
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amandeepsinghjamwal
changed the title
Bnyro
reviewed
May 14, 2024
Comment on lines
121
to
123
| } catch (e: Exception) { | ||
| isLoading = false | ||
| } |
There was a problem hiding this comment.
isLoading = false should probably be outside of the catch block, and not become called in fetchTabNextPage, just for readability
Bnyro
reviewed
May 14, 2024
Comment on lines
128
to
132
| val tabData = try { | ||
| Json.decodeFromString<ChannelTab>(arguments?.getString(IntentData.tabData)?:"") | ||
| } catch (e: Exception) { | ||
| null | ||
| } |
There was a problem hiding this comment.
Suggested change
| val tabData = try { | |
| Json.decodeFromString<ChannelTab>(arguments?.getString(IntentData.tabData)?:"") | |
| } catch (e: Exception) { | |
| null | |
| } | |
| val tabData = runCatching { | |
| Json.decodeFromString<ChannelTab>(arguments?.getString(IntentData.tabData).orEmpty()) | |
| }.getOrNull() |
Bnyro
reviewed
May 14, 2024
Comment on lines
145
to
148
| val visibleItemCount: Int = recyclerView.layoutManager!!.childCount | ||
| val totalItemCount: Int = recyclerView.layoutManager!!.getItemCount() | ||
| val firstVisibleItemPosition: Int = | ||
| (recyclerView.layoutManager as LinearLayoutManager).findFirstVisibleItemPosition() |
There was a problem hiding this comment.
Suggested change
| val visibleItemCount: Int = recyclerView.layoutManager!!.childCount | |
| val totalItemCount: Int = recyclerView.layoutManager!!.getItemCount() | |
| val firstVisibleItemPosition: Int = | |
| (recyclerView.layoutManager as LinearLayoutManager).findFirstVisibleItemPosition() | |
| val visibleItemCount = recyclerView.layoutManager!!.childCount | |
| val totalItemCount = recyclerView.layoutManager!!.getItemCount() | |
| val firstVisibleItemPosition = | |
| (recyclerView.layoutManager as LinearLayoutManager).findFirstVisibleItemPosition() |
There was a problem hiding this comment.
Honestly I copy pasted this code hehe that's why I forgot to refactor it because your code for bottom reached listener was misbehaving
fun RecyclerView.addOnBottomReachedListener(onBottomReached: () -> Unit) {
viewTreeObserver.addOnScrollChangedListener {
if (!canScrollVertically(1)) onBottomReached()
}
}
is misbehaving on second time you use it, instead of triggering when reached on bottom. It is triggered on each scroll. Atleast that was happening in my case. you might wanna check it out.
Bnyro
reviewed
May 14, 2024
Comment on lines
159
to
175
| if (!tabData?.data.isNullOrEmpty()) { | ||
| loadChannelTab(tabData ?: return) | ||
| } else { | ||
| val videoDataString = arguments?.getString(IntentData.videoList) | ||
| val videos = try { | ||
| Json.decodeFromString<List<StreamItem>>(videoDataString!!) | ||
| } catch (e: Exception) { | ||
| mutableListOf() | ||
| } | ||
| channelAdapter = VideosAdapter( | ||
| videos.toMutableList(), | ||
| forceMode = VideosAdapter.Companion.LayoutMode.CHANNEL_ROW | ||
| ) | ||
| binding.channelRecView.adapter = channelAdapter | ||
| binding.progressBar.visibility = View.GONE | ||
| binding.channelRecView.visibility = View.VISIBLE | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (!tabData?.data.isNullOrEmpty()) { | |
| loadChannelTab(tabData ?: return) | |
| } else { | |
| val videoDataString = arguments?.getString(IntentData.videoList) | |
| val videos = try { | |
| Json.decodeFromString<List<StreamItem>>(videoDataString!!) | |
| } catch (e: Exception) { | |
| mutableListOf() | |
| } | |
| channelAdapter = VideosAdapter( | |
| videos.toMutableList(), | |
| forceMode = VideosAdapter.Companion.LayoutMode.CHANNEL_ROW | |
| ) | |
| binding.channelRecView.adapter = channelAdapter | |
| binding.progressBar.visibility = View.GONE | |
| binding.channelRecView.visibility = View.VISIBLE | |
| } | |
| if (tabData?.data.isNullOrEmpty()) { | |
| val videoDataString = arguments?.getString(IntentData.videoList) | |
| val videos = runCatching { | |
| Json.decodeFromString<List<StreamItem>>(videoDataString!!) | |
| }.getOrElse { mutableListOf() } | |
| channelAdapter = VideosAdapter( | |
| videos.toMutableList(), | |
| forceMode = VideosAdapter.Companion.LayoutMode.CHANNEL_ROW | |
| ) | |
| binding.channelRecView.adapter = channelAdapter | |
| binding.progressBar.visibility = View.GONE | |
| binding.channelRecView.visibility = View.VISIBLE | |
| } else { | |
| loadChannelTab(tabData!!) | |
| } |
Bnyro
reviewed
May 14, 2024
Comment on lines
254
to
258
| fragment.arguments = Bundle().apply { | ||
| putString(IntentData.tabData, Json.encodeToString(list[position])) | ||
| putString(IntentData.videoList, Json.encodeToString(videos)) | ||
| putString(IntentData.channelId, channelId) | ||
| putString(IntentData.nextPage, nextPage) |
There was a problem hiding this comment.
Suggested change
| fragment.arguments = Bundle().apply { | |
| putString(IntentData.tabData, Json.encodeToString(list[position])) | |
| putString(IntentData.videoList, Json.encodeToString(videos)) | |
| putString(IntentData.channelId, channelId) | |
| putString(IntentData.nextPage, nextPage) | |
| fragment.arguments = bundleOf( | |
| IntentData.tabData to Json.encodeToString(list[position]) | |
| IntentData.videoList to Json.encodeToString(videos) | |
| IntentData.channelId to channelId | |
| IntentData.nextPage to nextPage |
Bnyro
reviewed
May 14, 2024
app/src/main/res/values/strings.xml
Outdated
Comment on lines
554
to
555
| <!-- TODO: Remove or change this placeholder text --> | ||
| <string name="hello_blank_fragment">Hello blank fragment</string> |
|
Thanks for your PR, looks great UI/UX-wise! I noticed you did a lot of things like you were writing pure Java, so I'd guess you're coming from a Java background :) Two general suggestions/things concerning this PR:
|
Bnyro
reviewed
May 14, 2024
Comment on lines
112
to
118
| fetchChannelNextPage(nextPage ?: return@launch).let { | ||
| nextPage = it | ||
| } | ||
| } else { | ||
| fetchTabNextPage(nextPage ?: return@launch, tab).let { | ||
| nextPage = it | ||
| } |
There was a problem hiding this comment.
Suggested change
| fetchChannelNextPage(nextPage ?: return@launch).let { | |
| nextPage = it | |
| } | |
| } else { | |
| fetchTabNextPage(nextPage ?: return@launch, tab).let { | |
| nextPage = it | |
| } | |
| nextPage = fetchChannelNextPage(nextPage ?: return@launch) | |
| } else { | |
| nextPage = fetchTabNextPage(nextPage ?: return@launch, tab) |
There was a problem hiding this comment.
Thank you @Bnyro for all the corrections, I will keep these in mind next time. Got to learn a lot from your corrections. I'll correct it in next commit
3 tasks
|
Thank you for your work on this PR! I've tried rebasing your code with some small code improvements which you seemed to have disabled unfortunately 😢, hence I merged the changes in #6053. Again, thanks for your PR 👍 |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds enhancement mentioned in issue #6008
VID_20240514113238.online-video-cutter.com.1.mp4