-
Notifications
You must be signed in to change notification settings - Fork 110
[CLX-3127][Horizon] Learner dashboard landscape mode #3364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
efcbf5e
23929d5
1bd9322
2755ea4
1a76a30
aefb7a3
f7d8fd6
a9cff12
3f03549
b4c005d
e24e554
0981b3d
378262d
5713fa7
ba12928
6629eff
321a631
c3f8437
9b5bc9d
9ec13d0
9a0fb2f
fa7d935
a6446fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,13 +22,19 @@ import android.content.pm.PackageManager | |
| import android.os.Build | ||
| import androidx.activity.compose.rememberLauncherForActivityResult | ||
| import androidx.activity.result.contract.ActivityResultContracts | ||
| import androidx.compose.foundation.horizontalScroll | ||
| import androidx.compose.foundation.layout.Arrangement | ||
| import androidx.compose.foundation.layout.BoxWithConstraints | ||
| import androidx.compose.foundation.layout.Column | ||
| import androidx.compose.foundation.layout.IntrinsicSize | ||
| import androidx.compose.foundation.layout.PaddingValues | ||
| import androidx.compose.foundation.layout.Row | ||
| import androidx.compose.foundation.layout.Spacer | ||
| import androidx.compose.foundation.layout.fillMaxWidth | ||
| import androidx.compose.foundation.layout.height | ||
| import androidx.compose.foundation.layout.heightIn | ||
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.foundation.layout.width | ||
| import androidx.compose.foundation.pager.rememberPagerState | ||
| import androidx.compose.foundation.rememberScrollState | ||
| import androidx.compose.foundation.verticalScroll | ||
|
|
@@ -76,6 +82,7 @@ import com.instructure.horizon.horizonui.foundation.HorizonColors | |
| import com.instructure.horizon.horizonui.foundation.HorizonElevation | ||
| import com.instructure.horizon.horizonui.foundation.HorizonSpace | ||
| import com.instructure.horizon.horizonui.foundation.SpaceSize | ||
| import com.instructure.horizon.horizonui.isWideLayout | ||
| import com.instructure.horizon.horizonui.molecules.Badge | ||
| import com.instructure.horizon.horizonui.molecules.BadgeContent | ||
| import com.instructure.horizon.horizonui.molecules.BadgeType | ||
|
|
@@ -191,42 +198,7 @@ fun DashboardScreen(uiState: DashboardUiState, mainNavController: NavHostControl | |
| refreshStateFlow | ||
| ) | ||
| HorizonSpace(SpaceSize.SPACE_16) | ||
| val pagerState = rememberPagerState{ 3 } | ||
| AnimatedHorizontalPager( | ||
| pagerState, | ||
| sizeAnimationRange = 0f, | ||
| contentPadding = PaddingValues(horizontal = 24.dp), | ||
| pageSpacing = 12.dp, | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { index, modifier -> | ||
| when (index) { | ||
| 0 -> { | ||
| DashboardMyProgressWidget( | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| modifier.padding(bottom = 16.dp) | ||
| ) | ||
| } | ||
| 1 -> { | ||
| DashboardTimeSpentWidget( | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| modifier.padding(bottom = 16.dp) | ||
| ) | ||
| } | ||
| 2 -> { | ||
| DashboardSkillOverviewWidget( | ||
| homeNavController, | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| modifier.padding(bottom = 16.dp) | ||
| ) | ||
| } | ||
| else -> { | ||
|
|
||
| } | ||
| } | ||
| } | ||
| NumericWidgetRow(shouldRefresh, refreshStateFlow, homeNavController) | ||
| DashboardSkillHighlightsWidget( | ||
| homeNavController, | ||
| shouldRefresh, | ||
|
|
@@ -303,6 +275,91 @@ private fun HomeScreenTopBar(uiState: DashboardUiState, mainNavController: NavCo | |
| } | ||
| } | ||
|
|
||
| @Composable | ||
| private fun NumericWidgetRow( | ||
| shouldRefresh: Boolean, | ||
| refreshStateFlow: MutableStateFlow<List<Boolean>>, | ||
| homeNavController: NavHostController | ||
| ) { | ||
| BoxWithConstraints { | ||
| val pagerState = rememberPagerState { 3 } | ||
| if (this.isWideLayout) { | ||
| Row( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.spacedBy(12.dp), | ||
| modifier = Modifier | ||
| .horizontalScroll(rememberScrollState()) | ||
| .fillMaxWidth() | ||
| .padding(horizontal = 24.dp) | ||
| .padding(bottom = 12.dp) | ||
| ) { | ||
| DashboardMyProgressWidget( | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| Modifier.width(IntrinsicSize.Max) | ||
| ) | ||
| DashboardTimeSpentWidget( | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| Modifier.width(IntrinsicSize.Max) | ||
| ) | ||
| DashboardSkillOverviewWidget( | ||
| homeNavController, | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| Modifier.width(IntrinsicSize.Max) | ||
| ) | ||
| } | ||
| } else { | ||
| AnimatedHorizontalPager( | ||
| pagerState, | ||
| sizeAnimationRange = 0f, | ||
| beyondViewportPageCount = 3, | ||
| contentPadding = PaddingValues(horizontal = 24.dp), | ||
| pageSpacing = 12.dp, | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { index, modifier -> | ||
| when (index) { | ||
| 0 -> { | ||
| DashboardMyProgressWidget( | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| modifier | ||
| .fillMaxWidth() | ||
| .padding(bottom = 16.dp) | ||
| ) | ||
| } | ||
|
|
||
| 1 -> { | ||
| DashboardTimeSpentWidget( | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| modifier | ||
| .fillMaxWidth() | ||
| .padding(bottom = 16.dp) | ||
| ) | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| 2 -> { | ||
| DashboardSkillOverviewWidget( | ||
| homeNavController, | ||
| shouldRefresh, | ||
| refreshStateFlow, | ||
| modifier | ||
| .fillMaxWidth() | ||
| .padding(bottom = 16.dp) | ||
| ) | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| else -> { | ||
|
|
||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Composable | ||
| private fun NotificationPermissionRequest() { | ||
| val context = LocalContext.current | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,14 @@ import java.util.Date | |
| data class DashboardPaginatedWidgetCardState( | ||
| val items: List<DashboardPaginatedWidgetCardItemState> = emptyList(), | ||
| val isLoading: Boolean = false, | ||
| ) | ||
| ) { | ||
| companion object { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| val Loading = DashboardPaginatedWidgetCardState( | ||
| items = listOf(DashboardPaginatedWidgetCardItemState.Loading), | ||
| isLoading = true | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| data class DashboardPaginatedWidgetCardItemState( | ||
| val chipState: DashboardPaginatedWidgetCardChipState? = null, | ||
|
|
@@ -15,7 +22,20 @@ data class DashboardPaginatedWidgetCardItemState( | |
| val date: Date? = null, | ||
| val title: String? = null, | ||
| val route: DashboardPaginatedWidgetCardButtonRoute? = null | ||
| ) | ||
| ) { | ||
| companion object { | ||
| val Loading = DashboardPaginatedWidgetCardItemState( | ||
| chipState = DashboardPaginatedWidgetCardChipState( | ||
| label = "Announcement", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these labels visible while loading?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, these are just placeholders to get the correct size. The skeleton loading indicator will be drawn instead. |
||
| color = StatusChipColor.Sky | ||
| ), | ||
| title = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Announcement title shown here.", | ||
| source = "Institution or Course Name Here", | ||
| date = Date(), | ||
| route = DashboardPaginatedWidgetCardButtonRoute.MainRoute("") | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| data class DashboardPaginatedWidgetCardChipState( | ||
| val label: String, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,10 +25,9 @@ import com.instructure.canvasapi2.utils.weave.tryLaunch | |
| import com.instructure.horizon.features.dashboard.DashboardEvent | ||
| import com.instructure.horizon.features.dashboard.DashboardEventHandler | ||
| import com.instructure.horizon.features.dashboard.DashboardItemState | ||
| import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardState | ||
| import com.instructure.horizon.features.dashboard.widget.course.card.CardClickAction | ||
| import com.instructure.horizon.features.dashboard.widget.course.card.DashboardCourseCardModuleItemState | ||
| import com.instructure.horizon.features.dashboard.widget.course.card.DashboardCourseCardState | ||
| import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardState | ||
| import com.instructure.horizon.model.LearningObjectType | ||
| import com.instructure.journey.type.ProgramProgressCourseEnrollmentStatus | ||
| import com.instructure.pandautils.utils.formatIsoDuration | ||
|
|
@@ -110,22 +109,7 @@ class DashboardCourseViewModel @Inject constructor( | |
| nextModuleForCourse = { courseId -> | ||
| fetchNextModuleState(courseId, forceNetwork) | ||
| }, | ||
| ).map { state -> | ||
| if (state.buttonState?.onClickAction is CardClickAction.Action) { | ||
| state.copy(buttonState = state.buttonState.copy( | ||
| onClickAction = CardClickAction.Action { | ||
| viewModelScope.tryLaunch { | ||
| updateCourseButtonState(state, isLoading = true) | ||
| state.buttonState.action() | ||
| onRefresh() | ||
| updateCourseButtonState(state, isLoading = false) | ||
| } catch { | ||
| updateCourseButtonState(state, isLoading = false) | ||
| } | ||
| }, | ||
| )) | ||
| } else state | ||
| } | ||
| ) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| val programCardStates = programs | ||
| .filter { program -> program.sortedRequirements.none { it.enrollmentStatus == ProgramProgressCourseEnrollmentStatus.ENROLLED } } | ||
|
|
@@ -156,22 +140,4 @@ class DashboardCourseViewModel @Inject constructor( | |
| onClickAction = CardClickAction.NavigateToModuleItem(courseId, nextModuleItem.id) | ||
| ) | ||
| } | ||
|
|
||
| private fun updateCourseButtonState(state: DashboardCourseCardState, isLoading: Boolean) { | ||
| _uiState.update { | ||
| it.copy( | ||
| courses = it.courses.map { originalState -> | ||
| if (originalState.title == state.title && originalState.parentPrograms == state.parentPrograms) { | ||
| originalState.copy( | ||
| buttonState = originalState.buttonState?.copy( | ||
| isLoading = isLoading | ||
| ) | ||
| ) | ||
| } else { | ||
| originalState | ||
| } | ||
| } | ||
| ) | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good refactoring to extract this logic into a separate composable! This improves code organization and makes the different layout strategies clearer.
Consider adding a
@Previewannotation for this composable to make it easier to test both layout variations visually during development.