[CLX-3127][Horizon] Learner dashboard landscape mode #3364
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR implements responsive layout improvements for the dashboard widgets, adding support for wide layouts (tablets/larger screens). Overall, this is a well-structured refactoring that improves the user experience on larger devices.
Strengths
✅ Excellent responsive design patterns - The use of BoxWithConstraints with isWideLayout extension provides a clean, reusable approach to responsive layouts
✅ Code organization - Good extraction of logic into separate composables (NumericWidgetRow, DashboardCourseCardWideContent, etc.)
✅ Consistent loading states - The introduction of companion object Loading states (e.g., DashboardCourseCardState.Loading) provides consistency across widgets
✅ Improved visual feedback - Better use of shimmer effects throughout loading states
✅ Code cleanup - Removed unused code (e.g., DashboardCourseCardLoading.kt, unused button state logic)
Areas for Consideration
🔍 Magic numbers - The 400.dp breakpoint and 320.dp image width should be extracted as design tokens/constants for easier maintenance
🔍 Performance - The beyondViewportPageCount = 3 in AnimatedHorizontalPager preloads all pages, which may impact performance
🔍 Localization - Verify that hardcoded minimum widths and placeholder text work well across all supported locales
🔍 Empty else branch - Line 353 in DashboardScreen.kt has an empty else that can be cleaned up
Testing Recommendations
- Test on various device sizes, especially foldables and tablets in different orientations
- Verify loading state transitions work smoothly
- Check accessibility with TalkBack on both compact and wide layouts
- Validate RTL layout support for the new wide layouts
Great work on improving the dashboard experience for larger screens! 🚀
| Modifier.width(IntrinsicSize.Max) | ||
| ) | ||
| DashboardSkillOverviewWidget( | ||
| homeNavController, |
There was a problem hiding this comment.
Good refactoring to extract this logic into a separate composable! This improves code organization and makes the different layout strategies clearer.
Consider adding a @Preview annotation for this composable to make it easier to test both layout variations visually during development.
| .fillMaxWidth() | ||
| .padding(bottom = 16.dp) | ||
| ) | ||
| } |
There was a problem hiding this comment.
The beyondViewportPageCount = 3 parameter preloads all 3 pages, which could impact performance if these widgets load heavy data or complex layouts. Consider whether this is necessary, or if a lower value (e.g., 1) would suffice while still providing smooth scrolling.
| .padding(bottom = 16.dp) | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Empty else branch can be removed for cleaner code. If this is intentional for future expansion, consider adding a comment explaining why it's here.
| val isLoading: Boolean = false, | ||
| ) | ||
| ) { | ||
| companion object { |
There was a problem hiding this comment.
Nice pattern using a companion object for loading states! This makes it easy to provide consistent loading UI across the app. Consider if these placeholder values should be localized or if they're intentionally non-localized as placeholder text.
| )) | ||
| } else state | ||
| } | ||
| ) |
There was a problem hiding this comment.
Removing the button state update logic simplifies the ViewModel significantly. However, I'm curious - was this functionality removed intentionally, or has it been moved elsewhere? If the button loading states are no longer needed, that's great. If they are needed, we should ensure they're handled appropriately.
|
|
||
| @Composable | ||
| private fun ProgramsText( | ||
| programs: List<DashboardCourseCardParentProgramState>, |
There was a problem hiding this comment.
Good improvement adding shimmerEffect to all the loading states! This provides better visual feedback. The use of isLoading parameter threading through all these components is clean.
One minor suggestion: Consider extracting the shimmer colors (especially for the institution-themed ones) into constants or the design system to ensure consistency.
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| modifier = modifier | ||
| .aspectRatio(1.69f) |
There was a problem hiding this comment.
Mixing isImageLoading and isLoading in the shimmer condition might lead to loading states persisting longer than intended. Consider whether both need to gate the shimmer effect, or if just isImageLoading is sufficient for the image component.
| } | ||
| } | ||
|
|
||
| val BoxWithConstraintsScope.isWideLayout |
There was a problem hiding this comment.
The 400.dp threshold for wide layout is a reasonable choice, but consider:
- Design consistency: Ensure this threshold aligns with your design system's breakpoint definitions
- Testing: Has this been tested across different device types (phones, tablets, foldables)?
- Naming: Consider if
isWideLayoutaccurately describes what this represents - perhapsisTabletLayoutorshouldUseWideLayoutmight be clearer?
Also, this is a great candidate for extracting into a design token constant rather than a magic number.
| ) | ||
|
|
||
| BoxWithConstraints { | ||
| if (this.isWideLayout) { |
There was a problem hiding this comment.
Great use of responsive layout here! The switch between Row and Column based on layout width is clean.
Note: In the Row layout, the skills use Modifier.weight(1f) which is good, but if the number of skills varies, you might want to handle cases where there are too many skills to fit comfortably in a row (e.g., by limiting display or using a scrollable row).
| modifier = Modifier | ||
| .shimmerEffect(isLoading) | ||
| .focusable() | ||
| .widthIn(min = 100.dp) |
There was a problem hiding this comment.
Good addition of widthIn(min = 100.dp) to prevent the time display from being too narrow. However, verify this works well with localized content in languages with longer number formats or different time representations.
📊 Code Coverage Report✅ Student
✅ Teacher
|
|
Student Install Page |
| companion object { | ||
| val Loading = DashboardPaginatedWidgetCardItemState( | ||
| chipState = DashboardPaginatedWidgetCardChipState( | ||
| label = "Announcement", |
There was a problem hiding this comment.
Are these labels visible while loading?
There was a problem hiding this comment.
No, these are just placeholders to get the correct size. The skeleton loading indicator will be drawn instead.
refs: CLX-3127
affects: Horizon
release note: none