Add tabbed product detail UI with new fragment and updated resources#849
Add tabbed product detail UI with new fragment and updated resources#849PruthiviRaj27 wants to merge 9 commits intomasterfrom
Conversation
|
@coderabbitai review |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a new tabbed product detail UI in the Android app by adding Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ProductDetailActivityV2
participant ProductDetailFragmentV2
participant ProductViewModel
participant TabFragments
User->>ProductDetailActivityV2: Opens product detail
ProductDetailActivityV2->>ProductDetailFragmentV2: Show fragment with productId
ProductDetailFragmentV2->>ProductViewModel: Initialize with productId
ProductViewModel-->>ProductDetailFragmentV2: Observe product data
ProductDetailFragmentV2->>TabFragments: Set up tabs (Description, Curriculum, Subjects, FAQ)
User->>TabFragments: Switches tabs
TabFragments-->>ProductDetailFragmentV2: Display corresponding content
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
store/src/main/res/layout/testpress_product_details_description_fragment.xml (1)
10-17: Consider replacing hardcoded values with resources.The layout contains two hardcoded values that should be extracted to resource files:
- The placeholder text "Work" should be removed or replaced with a string resource
- The text color "#727272" should be defined in colors.xml
- android:text="Work" - android:textColor="#727272" /> + android:textColor="@color/description_text_color" />store/src/main/res/drawable/baseline_question_mark_24.xml (1)
1-5: Consider removing hardcoded tint from vector drawable.The vector drawable has a hardcoded tint of "#000000", which may interfere with the color selector's ability to properly tint the icon based on selection state.
-<vector android:height="24dp" android:tint="#000000" +<vector android:height="24dp" android:viewportHeight="24" android:viewportWidth="24" android:width="24dp" xmlns:android="http://schemas.android.com/apk/res/android"> <path android:fillColor="@android:color/white" android:pathData="M11.07,12.85c0.77,-1.39 2.25,-2.21 3.11,-3.44c0.91,-1.29 0.4,-3.7 -2.18,-3.7c-1.69,0 -2.52,1.28 -2.87,2.34L6.54,6.96C7.25,4.83 9.18,3 11.99,3c2.35,0 3.96,1.07 4.78,2.41c0.7,1.15 1.11,3.3 0.03,4.9c-1.2,1.77 -2.35,2.31 -2.97,3.45c-0.25,0.46 -0.35,0.76 -0.35,2.24h-2.89C10.58,15.22 10.46,13.95 11.07,12.85zM14,20c0,1.1 -0.9,2 -2,2s-2,-0.9 -2,-2c0,-1.1 0.9,-2 2,-2S14,18.9 14,20z"/> </vector>store/src/main/res/drawable/baseline_subject_24.xml (1)
1-5: Consider removing hardcoded tint and check icon consistency.The vector drawable has two considerations:
- The hardcoded tint of "#000000" may interfere with the color selector's ability to control the icon color.
- This icon has auto-mirroring enabled for RTL support - ensure all other tab icons have consistent auto-mirroring settings for proper internationalization.
-<vector android:autoMirrored="true" android:height="24dp" - android:tint="#000000" android:viewportHeight="24" +<vector android:autoMirrored="true" android:height="24dp" + android:viewportHeight="24" android:viewportWidth="24" android:width="24dp" xmlns:android="http://schemas.android.com/apk/res/android"> <path android:fillColor="@android:color/white" android:pathData="M14,17L4,17v2h10v-2zM20,9L4,9v2h16L20,9zM4,15h16v-2L4,13v2zM4,5v2h16L20,5L4,5z"/> </vector>store/src/main/res/layout/testpress_product_details_fragment_v2.xml (1)
81-87: Improve bottom button container layout.The button container lacks proper styling and layout properties to ensure good visual hierarchy.
<LinearLayout android:id="@+id/button_container" android:layout_width="match_parent" android:layout_height="wrap_content" android:layout_alignParentBottom="true" - android:orientation="horizontal"> + android:orientation="horizontal" + android:background="@android:color/white" + android:elevation="4dp" + android:padding="8dp">store/src/main/java/in/testpress/store/ui/ProductDetailFragmentV2.kt (2)
208-218: Remove commented out code.Keeping commented out code makes the codebase harder to maintain and understand. Either restore the code if needed or remove it completely.
-// override fun onCreateView( -// inflater: LayoutInflater, container: ViewGroup?, -// savedInstanceState: Bundle? -// ): View? { -// val textView = TextView(requireContext()) -// textView.text = "Product Overview" -// textView.textSize = 18f -// return NestedScrollView(requireContext()).apply { -// addView(textView) -// } -// }
104-115: Add documentation for the companion object methods.The
show()method lacks documentation, making it harder for other developers to understand its purpose and usage.companion object { private const val DEFAULT_PRODUCT_ID = -1 + + /** + * Creates and displays a ProductDetailFragmentV2 in the specified container. + * + * @param activity The host FragmentActivity + * @param containerViewId The ID of the view container where the fragment will be placed + * @param productId The ID of the product to display + */ fun show(activity: FragmentActivity, containerViewId: Int, productId: Int) { val fragment = ProductDetailFragmentV2().apply { arguments = Bundle().apply { putInt(ProductDetailsActivityV2.PRODUCT_ID, productId) } } activity.supportFragmentManager.beginTransaction() .replace(containerViewId, fragment) .commit() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
samples/src/main/java/in/testpress/samples/core/TestpressCoreSampleActivity.java(1 hunks)samples/src/main/java/in/testpress/samples/store/NavigationDrawerActivity.java(1 hunks)store/src/main/java/in/testpress/store/ui/ProductDetailActivityV2.kt(1 hunks)store/src/main/java/in/testpress/store/ui/ProductDetailFragmentV2.kt(1 hunks)store/src/main/res/color/tab_icon_color.xml(1 hunks)store/src/main/res/drawable/baseline_description_24.xml(1 hunks)store/src/main/res/drawable/baseline_menu_book_24.xml(1 hunks)store/src/main/res/drawable/baseline_question_mark_24.xml(1 hunks)store/src/main/res/drawable/baseline_subject_24.xml(1 hunks)store/src/main/res/layout/testpress_product_details_description_fragment.xml(1 hunks)store/src/main/res/layout/testpress_product_details_fragment_v2.xml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
samples/src/main/java/in/testpress/samples/core/TestpressCoreSampleActivity.java (1)
core/src/main/java/in/testpress/core/TestpressSdk.java (1)
TestpressSdk(23-344)
🔇 Additional comments (6)
store/src/main/res/drawable/baseline_description_24.xml (1)
1-5: LGTM: Well-formed vector drawable resource.The vector drawable follows Android best practices with appropriate dimensions, viewport settings, and path data. This icon appears to be part of the new tabbed UI implementation.
store/src/main/res/layout/testpress_product_details_description_fragment.xml (1)
1-19: LGTM: Good use of NestedScrollView for scrollable content.The layout is well-structured using a NestedScrollView with appropriate scrolling behavior and padding, which works well for displaying rich text content like product descriptions.
store/src/main/res/drawable/baseline_menu_book_24.xml (1)
1-8: LGTM: Well-formed vector drawable with RTL support.The vector drawable is correctly implemented with proper dimensions, viewport settings, and path data. Good use of the
autoMirroredattribute for RTL language support.samples/src/main/java/in/testpress/samples/core/TestpressCoreSampleActivity.java (1)
113-114: Hardcoded credentials in sample code.While this is a sample app, hardcoded credentials could potentially pose a security risk if they're real credentials. Consider using environment variables or a secure configuration approach if this code might be used beyond demonstration purposes.
Are these generic test credentials specific to the demo environment, or should they be masked/secured?
store/src/main/java/in/testpress/store/ui/ProductDetailActivityV2.kt (1)
21-21: Successful integration of the new tabbed UI.The code now correctly uses
ProductDetailFragmentV2while maintaining the same interface parameters, ensuring a smooth transition to the new tabbed product detail UI.store/src/main/res/color/tab_icon_color.xml (1)
1-5: Well-implemented color selector for tab icons.This color selector follows Android best practices by using theme attributes for the selected state and a predefined color resource for the default state, ensuring consistent visual feedback across the tabbed interface.
| import in.testpress.samples.R; | ||
| import in.testpress.samples.core.TestpressCoreSampleActivity; | ||
| import in.testpress.store.TestpressStore; | ||
| import in.testpress.store.ui.ProductDetailFragmentV2; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the usage of this imported class.
The ProductDetailFragmentV2 class is imported but doesn't appear to be used in this file. If it's intended for future implementation, this is fine. Otherwise, consider removing unused imports.
🏁 Script executed:
#!/bin/bash
# Check if ProductDetailFragmentV2 is used anywhere in the NavigationDrawerActivity
rg -A 3 -B 3 "ProductDetailFragmentV2" samples/src/main/java/in/testpress/samples/store/NavigationDrawerActivity.java
# Check for usage in other files that might be extended from or related to this activity
rg "ProductDetailFragmentV2.*NavigationDrawerActivity" --include "*.java" --include "*.kt"Length of output: 989
Remove unused import of ProductDetailFragmentV2
The import of in.testpress.store.ui.ProductDetailFragmentV2 in NavigationDrawerActivity.java isn’t referenced elsewhere in this file and can be safely removed to clean up unused imports.
• File: samples/src/main/java/in/testpress/samples/store/NavigationDrawerActivity.java
• Line: 12
| InstituteSettings instituteSettings = new InstituteSettings("https://lmsdemo.testpress.in"); | ||
| instituteSettings.setWhiteLabeledHostUrl("https://lmsdemo.testpress.in"); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
URL endpoint update to demo environment.
The institute URL has been updated from sandbox to demo environment. This change appears intentional and aligned with the hardcoded credentials update.
Verify that other parts of the application that may be using the sandbox URL have also been updated accordingly:
🏁 Script executed:
#!/bin/bash
# Check for any remaining references to sandbox.testpress.in
rg "sandbox\.testpress\.in" --type-not xmlLength of output: 9767
Remaining sandbox URLs in tests need updating
We’ve verified that while the sample activity now points to the demo endpoint, numerous test fixtures and test classes still reference sandbox.testpress.in. These should be updated to the demo URL (or made configurable) to keep environments consistent:
• store/src/test/resources/products_list.json
• core/src/test/resources/discussions.json
• course/src/test/resources/content_attempts.json
• exam/src/test/java/in/testpress/exam/ui/AttemptActivityTest.java
• exam/src/androidTest/java/in/testpress/exam/ui/ReviewStatsActivityTest.java
• exam/src/androidTest/java/in/testpress/exam/ui/ShareToUnlockActivityTest.kt
• course/src/test/java/in/testpress/course/network/CourseNetworkTest.kt
Please replace all remaining sandbox.testpress.in references with the demo endpoint or refactor to use environment variables.
| android:text="Sociology Test Bundle Sociology Test Bundle" | ||
| android:textColor="@color/testpress_black" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded text with string resources.
The product title is hardcoded in the layout. This makes localization difficult and violates Android best practices.
- android:text="Sociology Test Bundle Sociology Test Bundle"
+ android:text="@string/product_title_placeholder"| <Button | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:text="Buy now"/> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ID and styling to Buy button for functionality.
The Buy button is missing an ID and proper styling, making it impossible to attach click listeners or style consistently.
<Button
+ android:id="@+id/buy_button"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
- android:text="Buy now"/>
+ android:text="@string/buy_now"
+ android:layout_margin="16dp"
+ style="@style/Widget.AppCompat.Button.Colored"/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:text="Buy now"/> | |
| <Button | |
| android:id="@+id/buy_button" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:text="@string/buy_now" | |
| android:layout_margin="16dp" | |
| style="@style/Widget.AppCompat.Button.Colored"/> |
| <TextView | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:text="300.00"/> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing ID and proper styling for the price TextView.
The price TextView lacks an ID, making it impossible to reference from code. It also has hardcoded text and no styling.
<TextView
+ android:id="@+id/price"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
- android:text="300.00"/>
+ android:text="@string/price_placeholder"
+ android:textSize="18sp"
+ android:textColor="@color/testpress_black"
+ android:layout_weight="1"
+ android:layout_margin="16dp"
+ android:layout_gravity="center_vertical"/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <TextView | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:text="300.00"/> | |
| <TextView | |
| android:id="@+id/price" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:text="@string/price_placeholder" | |
| android:textSize="18sp" | |
| android:textColor="@color/testpress_black" | |
| android:layout_weight="1" | |
| android:layout_margin="16dp" | |
| android:layout_gravity="center_vertical"/> |
| imageUrl, binding.productThumbnail, ImageUtils.getPlaceholdersOption() | ||
| ) | ||
| binding.title.text = domainProduct?.product?.title | ||
| //binding.price.text = String.format("₹%s", domainProduct?.product?.price) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment and implement the price display.
The price display is commented out, leaving the price TextView in the layout without updated content.
- //binding.price.text = String.format("₹%s", domainProduct?.product?.price)
+ binding.price.text = String.format("₹%s", domainProduct?.product?.price)| class ProductCurriculumFragment : Fragment() { | ||
| override fun onCreateView( | ||
| inflater: LayoutInflater, container: ViewGroup?, | ||
| savedInstanceState: Bundle? | ||
| ): View? { | ||
| val textView = TextView(requireContext()) | ||
| textView.text = "Curriculum Content" | ||
| textView.textSize = 18f | ||
| return NestedScrollView(requireContext()).apply { | ||
| addView(textView) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class ProductSubjectsFragment : Fragment() { | ||
| override fun onCreateView( | ||
| inflater: LayoutInflater, container: ViewGroup?, | ||
| savedInstanceState: Bundle? | ||
| ): View? { | ||
| val textView = TextView(requireContext()) | ||
| textView.text = "Subjects" | ||
| textView.textSize = 18f | ||
| return NestedScrollView(requireContext()).apply { | ||
| addView(textView) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class ProductFAQFragment : Fragment() { | ||
| override fun onCreateView( | ||
| inflater: LayoutInflater, container: ViewGroup?, | ||
| savedInstanceState: Bundle? | ||
| ): View? { | ||
| val textView = TextView(requireContext()) | ||
| textView.text = "FAQ" | ||
| textView.textSize = 18f | ||
| return NestedScrollView(requireContext()).apply { | ||
| addView(textView) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Implement actual content for the placeholder tab fragments.
Three of the four tab fragments contain only placeholder text instead of actual implementation. This makes the UI incomplete.
The fragments for Course Curriculum, Subjects, and FAQ should be implemented with actual content and functionality rather than simple placeholder text. Consider creating proper layouts for these fragments and implementing data loading similar to the Description fragment.
🏁 Script executed:
#!/bin/bash
# Check if there are any layout files related to these tab fragments
fd -e xml "curriculum|subjects|faq" --exec echo Found potential tab layout: {}Length of output: 82
Action Required: Replace placeholder tab fragments with real layouts and data
Our verification confirmed that ProductCurriculumFragment, ProductSubjectsFragment, and ProductFAQFragment in
store/src/main/java/in/testpress/store/ui/ProductDetailFragmentV2.kt
only inflate a TextView (no XML layouts exist), so they remain placeholders.
• File: ProductDetailFragmentV2.kt
– ProductCurriculumFragment
– ProductSubjectsFragment
– ProductFAQFragment
Please:
- Create dedicated XML layout files (e.g.,
fragment_curriculum.xml,fragment_subjects.xml,fragment_faq.xml). - Inflate these layouts in
onCreateViewinstead of the current TextView placeholder. - Implement data loading and UI binding (using ViewModels or existing data flows), mirroring the approach in the Description fragment.
| val html = Html.fromHtml( | ||
| product.descriptionHtml, | ||
| UILImageGetter(binding.description, requireActivity()), | ||
| null | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use non-deprecated HTML parsing method with proper flag.
The current code uses a deprecated version of Html.fromHtml without specifying the proper flag.
- val html = Html.fromHtml(
- product.descriptionHtml,
- UILImageGetter(binding.description, requireActivity()),
- null
- )
+ val html = if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.N) {
+ Html.fromHtml(
+ product.descriptionHtml,
+ Html.FROM_HTML_MODE_COMPACT,
+ UILImageGetter(binding.description, requireActivity()),
+ null
+ )
+ } else {
+ @Suppress("DEPRECATION")
+ Html.fromHtml(
+ product.descriptionHtml,
+ UILImageGetter(binding.description, requireActivity()),
+ null
+ )
+ }| private var productId: Int = DEFAULT_PRODUCT_ID | ||
| private var domainProduct: DomainProduct? = null | ||
|
|
||
| private val tabTitles = listOf("Description", "Course Curriculum", "Subjects", "FAQ") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use string resources for tab titles instead of hardcoded strings.
Hardcoded strings make localization difficult and violate Android best practices.
- private val tabTitles = listOf("Description", "Course Curriculum", "Subjects", "FAQ")
+ private val tabTitles = listOf(
+ getString(R.string.description),
+ getString(R.string.course_curriculum),
+ getString(R.string.subjects),
+ getString(R.string.faq)
+ )Committable suggestion skipped: line range outside the PR's diff.
| when (resource?.status) { | ||
| Status.LOADING -> {} | ||
| Status.SUCCESS -> { | ||
| this.domainProduct = resource.data | ||
| renderProductDetails() | ||
| } | ||
| Status.ERROR -> {} | ||
| else -> Unit | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement loading and error states in the UI.
The code has empty blocks for loading and error states, providing no feedback to users when data is loading or errors occur.
when (resource?.status) {
Status.LOADING -> {
+ // Show loading indicator
+ binding.progressBar.isVisible = true
}
Status.SUCCESS -> {
+ binding.progressBar.isVisible = false
this.domainProduct = resource.data
renderProductDetails()
}
Status.ERROR -> {
+ binding.progressBar.isVisible = false
+ // Show error message
+ showErrorView(resource.error?.message ?: getString(R.string.error_loading_product))
}
else -> Unit
}Consider adding a ProgressBar to your layout and an error view container.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| when (resource?.status) { | |
| Status.LOADING -> {} | |
| Status.SUCCESS -> { | |
| this.domainProduct = resource.data | |
| renderProductDetails() | |
| } | |
| Status.ERROR -> {} | |
| else -> Unit | |
| } | |
| when (resource?.status) { | |
| Status.LOADING -> { | |
| // Show loading indicator | |
| binding.progressBar.isVisible = true | |
| } | |
| Status.SUCCESS -> { | |
| // Hide loading indicator | |
| binding.progressBar.isVisible = false | |
| this.domainProduct = resource.data | |
| renderProductDetails() | |
| } | |
| Status.ERROR -> { | |
| // Hide loading indicator | |
| binding.progressBar.isVisible = false | |
| // Show error message | |
| showErrorView( | |
| resource.error?.message | |
| ?: getString(R.string.error_loading_product) | |
| ) | |
| } | |
| else -> Unit | |
| } |
1 participant
Changes done
Reason for the changes
Fixes # .
Summary by CodeRabbit
New Features
UI Improvements
Other Changes