-
Notifications
You must be signed in to change notification settings - Fork 61
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(native-app): Notifications final improvements #15240
Conversation
WalkthroughThe changes across the files in this update primarily introduce enhanced notification handling for the native app. Specifically, new messages and translations for empty notification lists are added, logic is implemented to display custom empty state visuals in the notifications screen, and minor UI improvements are applied. The alterations include message updates, rendering adjustments for empty states, and styling enhancements for uniformity and improved user experience. Changes
Sequence Diagram(s)Below is a high-level sequence diagram visualizing the new notification flow when the notification list is empty. sequenceDiagram
participant User
participant App
participant NotificationsScreen
participant API
participant UIComponents
User ->> App: Opens Notification Screen
App ->> API: Fetch notifications
API -->> App: Returns notifications (empty)
App ->> NotificationsScreen: Pass empty notifications data
NotificationsScreen ->> UIComponents: Request to render empty state
UIComponents -->> NotificationsScreen: Render empty state with message and image
NotificationsScreen -->> User: Display empty notification screen
This diagram demonstrates the new control flow, detailing how the app handles an empty notification list by fetching data through the API and rendering a customized empty state in the UI. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
apps/native/app/src/screens/inbox/inbox.tsx (1)
Line range hint
51-51
: The redeclaration ofListItem
could lead to confusion and potential bugs. It's important to ensure that each type or component has a unique name unless they are explicitly intended to override each other.- type ListItem = - | { id: string; type: 'skeleton' | 'empty' } - | (DocumentV2 & { type: undefined }); + type InboxListItem = + | { id: string; type: 'skeleton' | 'empty' } + | (DocumentV2 & { type: undefined });
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- apps/native/app/src/messages/en.ts (1 hunks)
- apps/native/app/src/messages/is.ts (1 hunks)
- apps/native/app/src/screens/inbox/inbox.tsx (1 hunks)
- apps/native/app/src/screens/notifications/notifications.tsx (8 hunks)
- apps/native/app/src/ui/lib/empty-state/empty-list.tsx (4 hunks)
Additional context used
Path-based instructions (5)
apps/native/app/src/ui/lib/empty-state/empty-list.tsx (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/notifications/notifications.tsx (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/inbox/inbox.tsx (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/messages/en.ts (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/messages/is.ts (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
Biome
apps/native/app/src/screens/inbox/inbox.tsx
[error] 51-51: Shouldn't redeclare 'ListItem'. Consider to delete it or rename it. (lint/suspicious/noRedeclare)
'ListItem' is defined here:
Additional comments not posted (17)
apps/native/app/src/ui/lib/empty-state/empty-list.tsx (4)
6-6
: ImportingTypography
from thetypography
module is a good practice as it ensures consistency across the UI components.
15-15
: The use of a theme-basedmargin-top
is a good approach for maintaining consistent spacing across different themes.
40-41
: The styling ofTitle
andDescription
usingTypography
ensures that text elements adhere to the overall design system. The centralized text alignment is a typical requirement for empty states, enhancing readability.Also applies to: 45-45
69-69
: Using thevariant
prop to specify typography styles is a best practice as it leverages the typographic scale for consistency.apps/native/app/src/screens/notifications/notifications.tsx (10)
7-7
: IntroducingEmptyList
is appropriate for handling scenarios where there are no notifications to display.
15-20
: The addition ofActivityIndicator
,FlatList
,SafeAreaView
,View
, andImage
from 'react-native' supports the new features in the notifications screen, such as displaying loading indicators and handling empty states.
47-47
: Proper management of asset paths ensures that resources are correctly resolved and used within the app.
58-58
: Adjusting themargin-bottom
to use theme spacing ensures consistent spacing across different themes and screen sizes.
63-65
: Defining a constant for the fallback icon URL is good practice for maintainability and reusability.
77-80
: Defining a new typeEmptyItem
helps in handling empty states more explicitly within the notifications list.
143-146
: Handling an empty state by returning an array with a singleEmptyItem
is a clear and explicit approach to indicate no notifications.
209-227
: Rendering an empty state using theEmptyList
component when there are no notifications enhances the user experience by providing clear visual feedback.
236-240
: The use of a fallback URL for notification icons is a robust approach to ensure that a default icon is displayed when the sender's logo is unavailable.
275-310
: Moving the button rendering toListHeaderComponent
is a logical change that improves the structure and readability of the component.apps/native/app/src/screens/inbox/inbox.tsx (1)
220-220
: AddingunreadCount
to the state inuseInboxQuery
is necessary for managing the unread count functionality accurately.apps/native/app/src/messages/en.ts (1)
284-286
: Adding localized messages for the empty notifications list is essential for internationalization and improves the user experience by providing context-specific information in the user's language.apps/native/app/src/messages/is.ts (1)
418-420
: Translations for the empty notifications state are correctly added and seem to align with the feature's requirements.
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. Great job.
Datadog ReportBranch report: ✅ 0 Failed, 20254 Passed, 0 Skipped, 18m 7.25s Total Time |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15240 +/- ##
=======================================
Coverage 37.10% 37.10%
=======================================
Files 6448 6448
Lines 131455 131455
Branches 37574 37574
=======================================
Hits 48781 48781
Misses 82674 82674
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
* feat: buttons at top should not be sticky * feat: add empty state for notifications * fix: make sure to update unreadCount in inbox when fetching next page * feat: add fallback icon for notifications --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…15239) * Fix locale updates. Add cleanstring * update string clean * Update packagejson * Update query * Update query * Add lang option * Test * Fix * Add locale * Test name update * fix: force generated files * chore: nx format:write update dirty files * Revert test * Remove ff * fix: hash generated files * fix(j-s): Null Service Reference (#15247) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(native-app): Notifications final improvements (#15240) * feat: buttons at top should not be sticky * feat: add empty state for notifications * fix: make sure to update unreadCount in inbox when fetching next page * feat: add fallback icon for notifications --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(consultation-portal): KAM-2522: PowerBI report added (#15251) * fix: hash generated files (#15253) * fix: hash generated files * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> * fix: remove file * Update __config.mjs --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: lommi <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: Guðjón Guðjónsson <[email protected]> Co-authored-by: Þórey Jóna <[email protected]> Co-authored-by: Karl Arnar Ægisson <[email protected]>
…15239) * Fix locale updates. Add cleanstring * update string clean * Update packagejson * Update query * Update query * Add lang option * Test * Fix * Add locale * Test name update * fix: force generated files * chore: nx format:write update dirty files * Revert test * Remove ff * fix: hash generated files * fix(j-s): Null Service Reference (#15247) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(native-app): Notifications final improvements (#15240) * feat: buttons at top should not be sticky * feat: add empty state for notifications * fix: make sure to update unreadCount in inbox when fetching next page * feat: add fallback icon for notifications --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(consultation-portal): KAM-2522: PowerBI report added (#15251) * fix: hash generated files (#15253) * fix: hash generated files * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> * fix: remove file * Update __config.mjs --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: lommi <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: Guðjón Guðjónsson <[email protected]> Co-authored-by: Þórey Jóna <[email protected]> Co-authored-by: Karl Arnar Ægisson <[email protected]>
…15255) * fix(documents-v2): Add download URL to detail document v2 (#15194) * fix(service-portal): Update notification icon styling (#15219) * Update notification icon styling * Add option to remove button docline * fix(service-portal): Notifications - locale updates. Add cleanstring (#15239) * Fix locale updates. Add cleanstring * update string clean * Update packagejson * Update query * Update query * Add lang option * Test * Fix * Add locale * Test name update * fix: force generated files * chore: nx format:write update dirty files * Revert test * Remove ff * fix: hash generated files * fix(j-s): Null Service Reference (#15247) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(native-app): Notifications final improvements (#15240) * feat: buttons at top should not be sticky * feat: add empty state for notifications * fix: make sure to update unreadCount in inbox when fetching next page * feat: add fallback icon for notifications --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(consultation-portal): KAM-2522: PowerBI report added (#15251) * fix: hash generated files (#15253) * fix: hash generated files * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> * fix: remove file * Update __config.mjs --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: lommi <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: Guðjón Guðjónsson <[email protected]> Co-authored-by: Þórey Jóna <[email protected]> Co-authored-by: Karl Arnar Ægisson <[email protected]> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: lommi <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: Guðjón Guðjónsson <[email protected]> Co-authored-by: Þórey Jóna <[email protected]> Co-authored-by: Karl Arnar Ægisson <[email protected]>
What
Checklist:
Summary by CodeRabbit
New Features
Enhancements
Typography
components for a consistent look.UI Adjustments