-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: Course Level Error Handling for Empty States #363
feat: Course Level Error Handling for Empty States #363
Conversation
- Empty state for Home, Videos, Dates, and Discussion tabs. - Empty state for handouts, and Announcements screen. fixes: LEARNER-10039
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.
Some minor code improvements.
@@ -0,0 +1,11 @@ | |||
<vector xmlns:android="http://schemas.android.com/apk/res/android" |
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.
Requires auto-format.
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.
we can move these icons in core.
@@ -0,0 +1,11 @@ | |||
<vector xmlns:android="http://schemas.android.com/apk/res/android" |
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.
we also have core_ic_no_content
for this purpose.
} | ||
DatesUIState.Error -> { | ||
NoContentScreen( | ||
message = stringResource(id = R.string.course_dates_unavailable_message), |
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.
can we update the string to
The course dates are currently not available.
@@ -5,5 +5,6 @@ import org.openedx.discussion.domain.model.Topic | |||
|
|||
sealed class DiscussionTopicsUIState { | |||
data class Topics(val data: List<Topic>) : DiscussionTopicsUIState() | |||
object Loading : DiscussionTopicsUIState() | |||
data object Loading : DiscussionTopicsUIState() | |||
data object Error : DiscussionTopicsUIState() | |||
} |
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.
please add a line at EOF.
} catch (e: Exception) { | ||
if (e.isInternetError()) { | ||
_uiMessage.emit(UIMessage.SnackBarMessage(resourceManager.getString(R.string.core_error_no_connection))) | ||
} else { | ||
_uiMessage.emit(UIMessage.SnackBarMessage(resourceManager.getString(R.string.core_error_unknown_error))) | ||
} | ||
_uiState.value = DiscussionTopicsUIState.Error |
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.
it will override the snackbar msg.
please update accordingly.
course/src/main/java/org/openedx/course/presentation/outline/CourseOutlineScreen.kt
Show resolved
Hide resolved
NoContentScreen( | ||
stringResource(id = R.string.course_no_videos), | ||
painterResource(id = R.drawable.course_ic_no_videos) | ||
) |
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.
Can we add progress for this case as well?
@@ -0,0 +1,11 @@ | |||
<vector xmlns:android="http://schemas.android.com/apk/res/android" |
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.
we can move this in core.
@@ -0,0 +1,11 @@ | |||
<vector xmlns:android="http://schemas.android.com/apk/res/android" |
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.
same here
@@ -253,6 +256,12 @@ private fun DiscussionTopicsUI( | |||
} | |||
|
|||
DiscussionTopicsUIState.Loading -> {} |
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.
Can we add CircularProgress for loading?
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.
only a minor nit.
rememberVectorPainter(image = Icons.Filled.Info) | ||
) | ||
} | ||
} |
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.
Please add a line at EOF.
@@ -38,6 +38,7 @@ | |||
<string name="discussion_unnamed_subcategory">Unnamed subcategory</string> | |||
|
|||
|
|||
|
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.
unused code
d8566c3
We're closing this PR and moving these changes to our own fork, as they are critical for us. After our release, we hope to find a way to contribute to these changes back, thanks! |
Description
Course Level Error Handling for Empty States
Tickets