[CLX-3253][Horizon] Dashboard widgets improvements #3371
Conversation
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
|
Student Install Page |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
This PR refactors the dashboard widget system to consolidate pagination state management and improve the visual consistency of widget cards. The changes move pagination display logic from individual widgets to a centralized DashboardWidgetCard component.
✅ Positive Aspects
- Good architectural improvement: Centralizing pagination state in
DashboardWidgetPageStatereduces code duplication and makes the pagination pattern consistent across all widgets - Separation of concerns: Moving pagination display logic from ViewModels to the UI layer is a cleaner separation
- Code consolidation: The refactoring eliminates the
DashboardAnnouncementBannerCardErrorcomponent and reuses the genericDashboardWidgetCardError, reducing maintenance burden - Loading state improvement: Using
DashboardPaginatedWidgetCardState.Loadingas a companion object provides a clean way to represent loading states - Text overflow handling: Adding
maxLinesandTextOverflow.Ellipsisprevents UI breaking with long text - Consistent styling: The header state consolidation (
DashboardPaginatedWidgetCardHeaderState) provides better type safety withColorinstead ofStatusChipColor
⚠️ Issues Found
- Non-exhaustive when expression in
DashboardScreen.kt:230- Removing theelsebranch could lead to silent failures if the pager index is out of range - Padding removal in
DashboardPaginatedWidgetCard.kt:106- Verify this is intentional and matches design specs - Visual breaking change in
DashboardWidgetCard.kt:96-120- The header layout order changed from Title-Icon to Icon-Title; ensure this matches design and update tests - Required parameter awkwardness in
DashboardWidgetCardError.kt:50-pageStateis required even for non-paginated contexts, consider making it optional - Source text truncation in
DashboardPaginatedWidgetCard.kt:130- 1 linemaxLinesfor source might be too restrictive for long course/institution names
🔍 Testing Recommendations
- Test with extremely long course/institution names to verify text truncation behavior
- Verify pagination numbers display correctly (1-indexed) across all widgets
- Test error states for both paginated and non-paginated widgets
- Verify accessibility with screen readers, especially with the header layout changes
- Update screenshot/UI tests for the visual changes
- Test edge cases with 1, 2, and 3+ pages in the horizontal pager
📝 Minor Suggestions
- Consider adding kdoc comments to
DashboardWidgetPageStateexplaining the 1-indexed convention - The
useMinWidthparameter in several places might benefit from a more descriptive name - Consider consistent ordering of optional parameters across similar functions
Overall, this is a solid refactoring that improves code maintainability. The main concerns are around ensuring visual regressions are caught and handling edge cases in the pagination logic.
| else -> { | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
The else branch was removed from the when expression, but this makes the when non-exhaustive. If the pager ever has more than 3 pages or an invalid index, this could lead to silent failures. Consider either keeping the empty else branch for safety, or adding a check/comment explaining why indices 0-2 are guaranteed.
| item = state.items[index], | ||
| item = item, | ||
| isLoading = state.isLoading, | ||
| modifier = Modifier |
There was a problem hiding this comment.
The hardcoded padding removal here (from 24.dp horizontal padding) changes the visual alignment of the content. Please verify this was intentional and consistent with the design. The DashboardWidgetCard wrapper should be handling padding consistently.
| Text( | ||
| text = stringResource( | ||
| R.string.dashboardAnnouncementBannerFrom, | ||
| source |
There was a problem hiding this comment.
Good improvement! Adding maxLines and overflow prevents text overflow issues. However, consider if 1 line is sufficient for course names which can be long. Might want to test with real data to ensure it doesn't truncate too aggressively.
| @DrawableRes iconRes: Int, | ||
| widgetColor: Color, | ||
| useMinWidth: Boolean, | ||
| pageState: DashboardWidgetPageState, |
There was a problem hiding this comment.
The new pageState parameter is required, but for error states where there's no pagination context (like single widgets), passing DashboardWidgetPageState.Empty feels awkward. Consider making this parameter optional with a default value of null or DashboardWidgetPageState.Empty.
|
Student Install Page |
|
Student Install Page |
refs: CLX-3253
affects: Horizon
release note: none