-
Notifications
You must be signed in to change notification settings - Fork 0
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: Notifications Inbox Screen #77
Conversation
Fixes: LEARNER-10346
Fixes: LEARNER-10346
Fixes: LEARNER-10346
Fixes: LEARNER-10346
Fixes: LEARNER-10346
f51b6de
to
8342325
Compare
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 in general, just some questions
notifications/src/main/java/org/openedx/notifications/data/api/APIConstants.kt
Outdated
Show resolved
Hide resolved
...ons/src/main/java/org/openedx/notifications/presentation/inbox/NotificationsInboxFragment.kt
Outdated
Show resolved
Hide resolved
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 concerns.
- Text color doesn't match with the Figma designs of static string like
responded to a post...
. - Pull to refresh should be implemented.
- No Quotation marks around the Post title.
- bold Text weight looks higher than the Figma designs.
Ago
word needs to be removed after the date as per the Fimga designs.- Sapce should be higher between notification text and date.
notifications/src/main/java/org/openedx/notifications/data/api/APIConstants.kt
Outdated
Show resolved
Hide resolved
notifications/src/main/java/org/openedx/notifications/data/api/NotificationsApi.kt
Show resolved
Hide resolved
notifications/src/main/java/org/openedx/notifications/data/model/InboxNotificationsResponse.kt
Show resolved
Hide resolved
notifications/src/main/java/org/openedx/notifications/data/model/InboxNotificationsResponse.kt
Outdated
Show resolved
Hide resolved
...fications/src/main/java/org/openedx/notifications/data/repository/NotificationsRepository.kt
Outdated
Show resolved
Hide resolved
...ons/src/main/java/org/openedx/notifications/presentation/inbox/NotificationsInboxFragment.kt
Show resolved
Hide resolved
...ons/src/main/java/org/openedx/notifications/presentation/inbox/NotificationsInboxFragment.kt
Outdated
Show resolved
Hide resolved
...ons/src/main/java/org/openedx/notifications/presentation/inbox/NotificationsInboxFragment.kt
Show resolved
Hide resolved
...ons/src/main/java/org/openedx/notifications/presentation/inbox/NotificationsInboxFragment.kt
Show resolved
Hide resolved
...ons/src/main/java/org/openedx/notifications/presentation/inbox/NotificationsInboxFragment.kt
Show resolved
Hide resolved
Fixes: LEARNER-10346
|
Fixes: LEARNER-10345
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.
Comments added for the pending points.
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.
LGTM 🔥 .
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.
LGTM
Description:
LEARNER-10346
A Notification Inbox Screen that organizes notifications into three categories: Recent (last 24 hours), This Week (past 7 days), and Older.
Screenshots:
Figma:
Mobile Notifications UI Screen