Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Copy link

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 @Preview annotation for this composable to make it easier to test both layout variations visually during development.

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)
)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


2 -> {
DashboardSkillOverviewWidget(
homeNavController,
shouldRefresh,
refreshStateFlow,
modifier
.fillMaxWidth()
.padding(bottom = 16.dp)
)
}

Copy link

Choose a reason for hiding this comment

The 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,14 @@ import java.util.Date
data class DashboardPaginatedWidgetCardState(
val items: List<DashboardPaginatedWidgetCardItemState> = emptyList(),
val isLoading: Boolean = false,
)
) {
companion object {
Copy link

Choose a reason for hiding this comment

The 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,
Expand All @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these labels visible while loading?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,15 @@ import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.unit.dp
import androidx.hilt.navigation.compose.hiltViewModel
import androidx.navigation.NavHostController
import com.instructure.horizon.R
import com.instructure.horizon.features.dashboard.DashboardItemState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCard
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardButtonRoute
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardChipState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardItemState
import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidgetCardState
import com.instructure.horizon.features.dashboard.widget.announcement.card.DashboardAnnouncementBannerCardError
import com.instructure.horizon.horizonui.molecules.StatusChipColor
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.update
import java.util.Date

@Composable
fun DashboardAnnouncementBannerWidget(
Expand Down Expand Up @@ -71,21 +65,7 @@ fun DashboardAnnouncementBannerSection(
when (state.state) {
DashboardItemState.LOADING -> {
DashboardPaginatedWidgetCard(
state.cardState.copy(
items = listOf(
DashboardPaginatedWidgetCardItemState(
chipState = DashboardPaginatedWidgetCardChipState(
label = stringResource(R.string.notificationsAnnouncementCategoryLabel),
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("")
)
),
isLoading = true
),
DashboardPaginatedWidgetCardState.Loading,
mainNavController,
homeNavController,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import com.instructure.horizon.features.dashboard.widget.DashboardPaginatedWidge
import com.instructure.horizon.features.dashboard.widget.course.card.CardClickAction
import com.instructure.horizon.features.dashboard.widget.course.card.DashboardCourseCardContent
import com.instructure.horizon.features.dashboard.widget.course.card.DashboardCourseCardError
import com.instructure.horizon.features.dashboard.widget.course.card.DashboardCourseCardLoading
import com.instructure.horizon.features.dashboard.widget.course.card.DashboardCourseCardState
import com.instructure.horizon.features.home.HomeNavigationRoute
import com.instructure.horizon.horizonui.foundation.HorizonColors
Expand Down Expand Up @@ -82,7 +81,12 @@ fun DashboardCourseSection(
) {
when(state.state) {
DashboardItemState.LOADING -> {
DashboardCourseCardLoading(Modifier.padding(horizontal = 24.dp))
DashboardCourseCardContent(
DashboardCourseCardState.Loading,
{ handleClickAction(it, mainNavController, homeNavController) },
true,
modifier = Modifier.padding(horizontal = 24.dp)
)
}
DashboardItemState.ERROR -> {
DashboardCourseCardError({state.onRefresh {} }, Modifier.padding(horizontal = 24.dp))
Expand Down Expand Up @@ -160,7 +164,9 @@ private fun DashboardCourseItem(
modifier = modifier.fillMaxWidth()
){
DashboardCourseCardContent(
cardState, { handleClickAction(it, mainNavController, homeNavController) }
cardState,
{ handleClickAction(it, mainNavController, homeNavController) },
false
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
)
Copy link

Choose a reason for hiding this comment

The 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 } }
Expand Down Expand Up @@ -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
}
}
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ private fun GetCoursesQuery.Enrollment.mapCompleted(context: Context, programs:
description = context.getString(R.string.dashboardCompletedCourseDetails),
progress = this.course?.usersConnection?.nodes?.firstOrNull()?.courseProgression?.requirements?.completionPercentage ?: 0.0,
moduleItem = null,
buttonState = null,
onClickAction = CardClickAction.NavigateToCourse(this.course?.id?.toLongOrNull() ?: -1L)
)
}
Expand All @@ -124,8 +123,6 @@ private suspend fun GetCoursesQuery.Enrollment.mapActive(
description = null,
progress = this.course?.usersConnection?.nodes?.firstOrNull()?.courseProgression?.requirements?.completionPercentage ?: 0.0,
moduleItem = nextModuleForCourse(this.course?.id?.toLongOrNull()),
buttonState = null,
onClickAction = CardClickAction.NavigateToCourse(this.course?.id?.toLongOrNull() ?: -1L),
lastAccessed = this.lastActivityAt
)
}
Loading
Loading