feat: Enable navigation from NotificationsInbox to Discussion #84
Conversation
omerhabib26
force-pushed
the
2U/omer/LEARNER-10373
branch
4 times, most recently
from
fa14fa1 to
06d81a9
Compare
omerhabib26
force-pushed
the
2U/omer/LEARNER-10373
branch
from
06d81a9 to
3279131
Compare
| comments.remove(comment) | ||
| comments.add(0, comment!!) | ||
| } | ||
| } |
There was a problem hiding this comment.
First: need to adjust formatting here
Second: comments.find {} is running 2 times here is some cases
| @@ -379,6 +397,10 @@ private fun DiscussionCommentsScreen( | |||
| onUserPhotoClick = { | |||
| onUserPhotoClick(comment.author) | |||
| }) | |||
| if (fromNotificationNavigation && commentId.isNotEmpty() && responseId == comment.id) { | |||
There was a problem hiding this comment.
maybe it's better to move this to Effect? It's strange for me to execute onCommentClick lambda inside the compose graph
| val index = | ||
| uiState.data.indexOfFirst { it.id == threadId } | ||
| if (index != -1) { | ||
| onItemClick(uiState.data[index]) |
There was a problem hiding this comment.
the same here, the onItemClick is executed in the middle of the compose function
There was a problem hiding this comment.
-
Error handling needs to be considered part of this PR.
Error Handling:
Manage cases where a notification links to a deleted or unavailable discussion, with clear messaging to the user. This should be a toast message.
Error copy: “Unable to retrieve comment/response.” -
IMO, its better to add the following properties in
Notification:Notification Tappedevent.course_idnotification_idtopic_idpost_idresponse_idcomment_id.
-
Need to eliminate the flickering effect during navigation if feasible with minimal effort (can be considered in Notification V2).
-
IMO, its better to navigate in the following order if feasible with minimal effort (can be considered in Notification V2)
Notification Inbox -> Course Dashboard -> Course Discussion tab -> All posts -> Discussion Detail Screen -> Response Details screen.
|
Error Handing on Notification Inbox screen is already handled, for discussion module we can handle it in a separate ticket.
There is a ticket in a backlog for analytic improvement, we can cover these suggestions in that ticket.
Yes, we can improve this in V2.
|
| try { | ||
| headers.forEach { | ||
| val startIndex = text.indexOf(it) | ||
| val endIndex = startIndex + it.length + 1 | ||
| result = text.replaceRange(startIndex, endIndex, it + "\n") | ||
| } | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() |
There was a problem hiding this comment.
use case for this code change?
There was a problem hiding this comment.
The server was passing an incorrect HTML string, which caused the exception and prevented the app from parsing the data.
| mutableStateOf(threadId.isNotEmpty()) | ||
| } | ||
|
|
||
| LaunchedEffect(uiState is DiscussionThreadsUIState.Threads) { |
There was a problem hiding this comment.
IMO better to use LaunchedEffect(uiState).
| var fromNotificationNavigation by rememberSaveable { | ||
| mutableStateOf(viewModel.responseId.isNotEmpty() && viewModel.commentId.isNotEmpty()) | ||
| } | ||
| LaunchedEffect(uiState is DiscussionCommentsUIState.Success) { |
There was a problem hiding this comment.
IMO better to use LaunchedEffect(uiState).
- Allows users to move from the NotificationsInbox screen to the Discussion component fix: LEARNER-10373
omerhabib26
force-pushed
the
2U/omer/LEARNER-10373
branch
from
8f4a00e to
e84a72d
Compare
Description:
fix: LEARNER-10373
PushNotificationsFlow.webm