chore: Notifications Inbox Screen Implementation #113

shafqat-muneer · 2025-01-09T09:06:55Z

LEARNER-10288: Notifications Inbox Screen Implementation

Description:

We need to implement a Notifications screen in the edX mobile app. This screen will display notifications fetched from the notifications table, allowing learners to stay updated on relevant discussions and interactions.

Design cases:

Empty screen when there are no notifications to show {It will be covered in separate ticket}
Distinguishable read and unread notifications
Grouping based on how recent the notification is (i.e. today/recent, last week, older)?

Design UI for the notifications screen. Main components include:

Icon for notification type (Discussions, Learning Reminder, Promotion) {Currently, just Discussions notifications implemented, so we are just showing discussions icon}
Ability to distinguish read and unread notifications (Unread indicator)
Time elapsed
Group notifications based on recency (Today/Recent, This Week, Older)
Ability to mark all notifications as read {It's dependent on this PR. API integrated there. We will handle this point in separate PR}

Tasks:
Notifications Screen UI:

We need to design and implement the layout for the Notifications screen which is the list view for displaying individual notifications. Ensure the UI is user-friendly, visually aligned with the app’s design standards, and optimized for different screen sizes.
The UI will include notification groups based on recency. The following is the logic to be implemented for these groups:
- Recent: within the last 24 hours
- This Week: greater than 24 hours but not more than 7 day old
- Older: more than 7 day old
The notifications also show the time elapsed on each notification since it was received. We need the time elapse to be human-readable similar to the dates tab. i.e. 5 minutes, 2 days, 1 week, 1 month, etc.
Icons on notifications depend on the notification category.

Acceptance Criteria:

A functional Notifications screen is implemented, displaying notifications from the notifications table in an organized list.

LEARNER-10356: Notifications marked as seen & read integration with Notifications Inbox Screen.

Mark notifications as seen
Once the inbox screen is opened the Mark as Seen API needs to be hit with app_name as discussion which would set the count in the API to 0

Single notification marked as read
When a learner taps on a single notification, the notification will be marked read, and the unread indicator should go away. The indicator is just the red dot on the notification.

Screenshots:

Light Mode	Dark Mode

Demo:

Light Mode	Dark Mode
Simulator.Screen.Recording.-.iPhone.16.-.2025-01-09.at.13.16.16.mp4	Simulator.Screen.Recording.-.iPhone.16.-.2025-01-09.at.13.16.32.mp4

Figma:

Mobile Notifications UI Screen

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxView.swift

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxViewModel.swift

Notifications/Notifications/Data/Network/NotificationsEndpoint.swift

Notifications/Notifications/Presentation/AttributedText/AttributedText.swift

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxView.swift

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxViewModel.swift

OpenEdX.xcodeproj/project.pbxproj

mta452 · 2025-01-09T14:19:09Z

Notifications/Notifications/Presentation/AttributedText/AttributedText.swift

+     .padding()
+ ```
+ */
+public struct AttributedText: View {


Have we considered implementing it based on the built in HTML support of NSAttributedString as a simple solution?

#113 (comment)

mta452 · 2025-01-09T14:20:07Z

Notifications/Notifications/Presentation/AttributedText/HTML2TextParser.swift

+    }
+
+    /// Starts the text parsing process. The results of this method will be placed in the `formattedText` variable.
+    internal func parse() {


Have we considered implementing this logic based on XMLParser instead of writing our own?

mta452 · 2025-01-10T06:09:48Z

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxViewModel.swift

+            if totalPages > 1 {
+                if index == flatNotifications.count - 3 {
+                    if totalPages != 1 {
+                        if nextPage <= totalPages {


This can be simplified to a single condition without nesting.

For easy understandable, I used this approach and it’s also already using at many places in codebase. Example: AllCoursesViewModel.swift

shafqat-muneer · 2025-01-14T09:28:37Z

@rnr @mta452 I've addressed the reviewed feedback. Please have another round of review.

mta452

Sorry, I hadn't checked the design details initially. Added relevant comments now.

Dashboard/Dashboard.xcodeproj/project.pbxproj

mta452 · 2025-01-14T10:55:23Z

Notifications/Notifications/Presentation/NotificationsInbox/SingleNotificationView.swift

+                    .accessibilityIdentifier("discussions_icon")
+            }
+            .frame(maxHeight: .infinity, alignment: .top)
+            .padding(.top, 5)


The reference design has a padding of 8px on both the top and the bottom of the row. So I would suggest to apply it on the upper HStack

Notifications/Notifications/Presentation/NotificationsInbox/SingleNotificationView.swift

mta452 · 2025-01-14T11:16:33Z

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxView.swift

+                            // Open three dots menu
+                        } label: {
+                            NotificationsAssets.threeDotsMenu.swiftUIImage
+                                .padding(.vertical, 10)


This should be 4px as per the reference design.

mta452 · 2025-01-14T11:17:41Z

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxView.swift

+                        } label: {
+                            NotificationsAssets.threeDotsMenu.swiftUIImage
+                                .padding(.vertical, 10)
+                                .padding(.horizontal, isHorizontal ? 48 : 25)


This should be 10px as per the reference design. It can also be simplified by setting a frame of 24x24 instead of applying padding.
Additional adjustment should be done on the button instead of the image in my opinion.

mta452 · 2025-01-14T11:27:10Z

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxView.swift

+                        .backViewStyle()
+                        .frame(width: 30, height: 30)
+                        .offset(y: 10)
+                        .padding(.leading, isHorizontal ? 48 : 10)


There is no need apply backViewStyle() since the styling is being done on top of it as well. Also the frame size is 24x24 in the reference design. There's no offset and the padding is 24px.

mta452 · 2025-01-14T11:29:48Z

Notifications/Notifications/Presentation/NotificationsInbox/NotificationsInboxView.swift

+                                            .foregroundColor(Theme.Colors.textSecondary)
+                                            .frame(maxWidth: .infinity, alignment: .leading)
+                                            .padding(.bottom, 5)
+                                            .padding(.horizontal, 20)


The padding should be 24 on all sides as per the design.

shafqat-muneer · 2025-01-16T18:21:23Z

@rnr @mta452 Review feedback addressed and also Mark notifications as seen and Single notification marked as read APIs integrated with Notification Inbox Screen. PR description is updated with details.

Notifications/Notifications/Data/Network/NotificationsEndpoint.swift

rnr · 2025-01-17T15:30:43Z

@rnr @mta452 Review feedback addressed and also Mark notifications as seen and Single notification marked as read APIs integrated with Notification Inbox Screen. PR description is updated with details.

@shafqat-muneer for "also Mark notifications as seen APIs integrated with Notification Inbox Screen" there should be menu in three-dot menu, right? Did it add too?

rnr · 2025-01-17T15:32:48Z

@shafqat-muneer Why Notifications text is greyed out now? Looks like disabled. Is it by design?
Ah I see it's better after the Theme implementation. It's looking disabled on default openedx Theme.
But anyway - in Figma I see color #5C5C5C for usual text and #19212F for 'bold' in Notifications Text. Theme.Colors.textSecondary doesn't look correct for this. Could you please check the colors? Thank you

rnr · 2025-01-17T15:49:39Z

Did we lose unread notifications mark here?
or
it's because of this?

Once the inbox screen is opened the Mark as Seen API needs to be hit with app_name as discussion which would set the count in the API to 0```

rnr · 2025-01-17T16:04:43Z

@shafqat-muneer If we follow this: "Once the inbox screen is opened the Mark as Seen API needs to be hit with app_name as discussion which would set the count in the API to 0" then what is the moment when we should hide unread mark on the Learn page? Now only re-run updates this mark

Screen.Recording.2025-01-17.at.17.00.55.mov

shafqat-muneer · 2025-01-17T16:36:12Z

@rnr @mta452 Review feedback addressed and also Mark notifications as seen and Single notification marked as read APIs integrated with Notification Inbox Screen. PR description is updated with details.

@shafqat-muneer for "also Mark notifications as seen APIs integrated with Notification Inbox Screen" there should be menu in three-dot menu, right? Did it add too?

@rnr There are three APIs:

Mark Notifications as Seen: When user will land on notifications inbox screen then this API will be called.
Mark Notifications as Read: When user will tap on any notification to navigate to specific discussion, we are calling this API upon tapping.
Mark All Notifications as Read: There will be three-dot-menu and there will be option for mark all as read. Once we will integrate menu then we will call this API. It's in a separate ticket scope.

In short, we have a separate ticket for the three-dot menu, and it will be addressed in a separate PR.

shafqat-muneer · 2025-01-17T17:08:07Z

@shafqat-muneer Why Notifications text is greyed out now? Looks like disabled. Is it by design? Ah I see it's better after the Theme implementation. It's looking disabled on default openedx Theme. But anyway - in Figma I see color #5C5C5C for usual text and #19212F for 'bold' in Notifications Text. `Theme.Colors.textSecondary` doesn't look correct for this. Could you please check the colors? Thank you

@rnr You're right; it will look better after applying the Theme. Currently, these values are not part of our existing Theme. I used a similar one that seemed appropriate. Do you have any suggestions for this based on our current theming colors?

I noticed CourseCardShadow is set to #19212F for a bold look but its dark mode variant will not be relevant. Also I couldn't find a color specifically for regular text. The one I used feels like the closest match to me for dark and light mode for our Theme.

Light	Dark

shafqat-muneer · 2025-01-17T17:12:42Z

Did we lose unread notifications mark here? or it's because of this?
Once the inbox screen is opened the Mark as Seen API needs to be hit with app_name as discussion which would set the count in the API to 0```

@rnr Yes, when the user lands on the Notification Inbox screen, we make the mark-seen API call. After that, the notifications count will be set to 0, and the unread marker will no longer be shown.

shafqat-muneer · 2025-01-17T17:19:46Z

@shafqat-muneer If we follow this: "Once the inbox screen is opened the Mark as Seen API needs to be hit with app_name as discussion which would set the count in the API to 0" then what is the moment when we should hide unread mark on the Learn page? Now only re-run updates this mark

Screen.Recording.2025-01-17.at.17.00.55.mov

@rnr That's correct. Currently, the notification marker updates on app relaunch or whenever the API is called. I think we should update it when the user navigates back from the Notification Inbox screen. We can handle this in a separate PR.

rnr · 2025-01-17T18:13:24Z

@shafqat-muneer If we follow this: "Once the inbox screen is opened the Mark as Seen API needs to be hit with app_name as discussion which would set the count in the API to 0" then what is the moment when we should hide unread mark on the Learn page? Now only re-run updates this mark
Screen.Recording.2025-01-17.at.17.00.55.mov

@rnr That's correct. Currently, the notification marker updates on app relaunch or whenever the API is called. I think we should update it when the user navigates back from the Notification Inbox screen. We can handle this in a separate PR.

Is there a reason why we need a separate PR to fix this bug (I guess we could call it a bug since we are sending an API request, but the UI is not updating)? We don't have product decision how it should work? Or it's tracked as bug somewhere?
Thank you

rnr · 2025-01-17T18:21:07Z

@shafqat-muneer Why Notifications text is greyed out now? Looks like disabled. Is it by design? Ah I see it's better after the Theme implementation. It's looking disabled on default openedx Theme. But anyway - in Figma I see color #5C5C5C for usual text and #19212F for 'bold' in Notifications Text. `Theme.Colors.textSecondary` doesn't look correct for this. Could you please check the colors? Thank you

@rnr You're right; it will look better after applying the Theme. Currently, these values are not part of our existing Theme. I used a similar one that seemed appropriate. Do you have any suggestions for this based on our current theming colors?

I noticed CourseCardShadow is set to #19212F for a bold look but its dark mode variant will not be relevant. Also I couldn't find a color specifically for regular text. The one I used feels like the closest match to me for dark and light mode for our Theme.

Light Dark

If we need to follow the design, we can create a new color in the whitelabel configuration. But here's a question for @moiz994 - is there a design reason why this screen is using new colors for the text rather than reusing existing colors?

shafqat-muneer · 2025-01-17T18:58:40Z

@shafqat-muneer If we follow this: "Once the inbox screen is opened the Mark as Seen API needs to be hit with app_name as discussion which would set the count in the API to 0" then what is the moment when we should hide unread mark on the Learn page? Now only re-run updates this mark
Screen.Recording.2025-01-17.at.17.00.55.mov

@rnr That's correct. Currently, the notification marker updates on app relaunch or whenever the API is called. I think we should update it when the user navigates back from the Notification Inbox screen. We can handle this in a separate PR.

Is there a reason why we need a separate PR to fix this bug (I guess we could call it a bug since we are sending an API request, but the UI is not updating)? We don't have product decision how it should work? Or it's tracked as bug somewhere? Thank you

I thought this should ideally have been part of the original PR that implemented the API integration, but since that PR is already merged, creating a separate PR for it makes sense. However, you're also right that this can also be considered a bug. Let's fix it in the current PR to avoid confusion. Thanks!

shafqat-muneer · 2025-01-17T19:39:04Z

I thought this should ideally have been part of the original PR that implemented the API integration, but since that PR is already merged, creating a separate PR for it makes sense. However, you're also right that this can also be considered a bug. Let's fix it in the current PR to avoid confusion. Thanks!

@rnr It's done.

rnr · 2025-01-20T12:37:50Z

Dashboard/Dashboard/Presentation/PrimaryCourseDashboardView.swift

@@ -333,6 +335,7 @@ public struct PrimaryCourseDashboardView<ProgramView: View>: View {
                        Spacer()
                        if viewModel.config.pushNotificationsEnabled {
                            Button(action: {
+                                viewModel.setNotificationMarkAsRead()


@shafqat-muneer Is it acceptable when an error appears on the Notifications Inbox View and the user cannot “read” the notifications (but we have already hidden the unread mark)?

@rnr When user returns from Inbox screen, we make /api/notifications/count/ API call to get actual count for the following reasons:

A new notification might arrive while the user is on the notifications inbox screen. When they return, the unread marker will update.

We also make an API call when switching tabs.

In case of an error, unread marker will update automatically.

Awesome. Thank you

chore: Notifications Inbox Screen Implementation

fa0dac1

shafqat-muneer requested review from rnr and mta452 January 9, 2025 09:06

This comment was marked as resolved.

Sign in to view

rnr reviewed Jan 9, 2025

View reviewed changes

mta452 reviewed Jan 10, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

refactor: address review feedback

c9f7fc3

This comment was marked as resolved.

Sign in to view

mta452 reviewed Jan 14, 2025

View reviewed changes

shafqat-muneer added 2 commits January 15, 2025 10:14

chore: resolve conflicts

e71870d

chore: address review feedback

26ba1d2

shafqat-muneer added 2 commits January 16, 2025 23:32

chore: address review feedback

f193c72

chore: address review feedback

d799d7f

rnr reviewed Jan 17, 2025

View reviewed changes

Notifications/Notifications/Data/Network/NotificationsEndpoint.swift Outdated Show resolved Hide resolved

mta452 approved these changes Jan 17, 2025

View reviewed changes

chore: address review feedback

4d26d22

chore: notificatoin marker update

33d7a79

rnr approved these changes Jan 20, 2025

View reviewed changes

fix: update notification text and icon color

26adbdc

shafqat-muneer merged commit f2bad20 into 2U/develop Jan 21, 2025
2 checks passed

shafqat-muneer deleted the Shafqat/LEARNER-10288 branch January 21, 2025 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Notifications Inbox Screen Implementation #113

chore: Notifications Inbox Screen Implementation #113

shafqat-muneer commented Jan 9, 2025 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

mta452 Jan 9, 2025

shafqat-muneer Jan 14, 2025

mta452 Jan 9, 2025

mta452 Jan 10, 2025

shafqat-muneer Jan 14, 2025

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

shafqat-muneer commented Jan 14, 2025

mta452 left a comment

mta452 Jan 14, 2025

mta452 Jan 14, 2025

mta452 Jan 14, 2025

mta452 Jan 14, 2025

mta452 Jan 14, 2025

shafqat-muneer commented Jan 16, 2025 •

edited

Loading

rnr commented Jan 17, 2025

rnr commented Jan 17, 2025 •

edited

Loading

rnr commented Jan 17, 2025 •

edited

Loading

rnr commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

rnr commented Jan 17, 2025

rnr commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

rnr Jan 20, 2025

shafqat-muneer Jan 20, 2025 •

edited

Loading

rnr Jan 20, 2025

chore: Notifications Inbox Screen Implementation #113

chore: Notifications Inbox Screen Implementation #113

Conversation

shafqat-muneer commented Jan 9, 2025 • edited Loading

Description:

Acceptance Criteria:

Screenshots:

Demo:

Figma:

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

shafqat-muneer commented Jan 14, 2025

mta452 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shafqat-muneer commented Jan 16, 2025 • edited Loading

rnr commented Jan 17, 2025

rnr commented Jan 17, 2025 • edited Loading

rnr commented Jan 17, 2025 • edited Loading

rnr commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

rnr commented Jan 17, 2025

rnr commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

shafqat-muneer commented Jan 17, 2025

Choose a reason for hiding this comment

shafqat-muneer Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shafqat-muneer commented Jan 9, 2025 •

edited

Loading

shafqat-muneer commented Jan 16, 2025 •

edited

Loading

rnr commented Jan 17, 2025 •

edited

Loading

rnr commented Jan 17, 2025 •

edited

Loading

shafqat-muneer Jan 20, 2025 •

edited

Loading