BA-2392 RN Notifications: Divide List by Read #243
Conversation
tsl-ps2
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
WalkthroughThe changes introduce a new functional component named Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/notifications/native/NotificationsList/Divider/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/notifications/native/NotificationsList/Divider/styles.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/modules/notifications/native/NotificationsList/Divider/index.tsx (1)
16-18: Consider adding accessibility support to the divider text.For better accessibility, consider adding appropriate accessibility props to the Text component to ensure screen readers can properly announce the divider.
<View> - <Text variant="caption">Read</Text> + <Text variant="caption" accessibilityRole="header" accessibilityLabel="Read notifications section">Read</Text> </View>packages/components/modules/notifications/native/NotificationsList/index.tsx (1)
55-73: Logic for displaying dividers between read and unread notifications is well implemented.The code now correctly checks if the current notification is read and the previous one is unread to determine when to show the divider. The React Fragment is appropriately used to render multiple elements.
However, there's still a TODO comment on line 61 that seems redundant given that you've now implemented the divider functionality.
- // TODO add notifications divider and unread/Read notifications as per design
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/components/modules/notifications/native/NotificationsList/Divider/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/Divider/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(2 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/components/modules/notifications/native/NotificationsList/Divider/index.tsx (1)
packages/components/modules/notifications/native/NotificationsList/Divider/styles.ts (1) (1)
createStyles(5-25)
packages/components/modules/notifications/native/NotificationsList/index.tsx (1)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts (1) (1)
NotificationItemProps(9-13)
🔇 Additional comments (9)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts (1)
12-12: Appropriate addition of refetch callback to support dynamic updates.Adding the
refetchproperty to theNotificationItemPropsinterface enables the component to trigger data refreshes when a notification is marked as read, which aligns with the requirement to dynamically move notifications between read/unread groups without page redirects.packages/components/modules/notifications/native/NotificationsList/Divider/styles.ts (1)
1-25: Well-structured and flexible styling for the divider component.The styles are defined using a function that accepts a theme parameter, which promotes consistent theming across the application. The layout uses flexbox appropriately to create a horizontal divider with text in the center.
packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx (1)
13-13: Good implementation of the refetch callback.The component now correctly accepts the
refetchprop and uses it in theonCompletedcallback of the mutation to refresh the notification list after marking a notification as read. This enables the UI to update dynamically when notifications change status.Also applies to: 22-22
packages/components/modules/notifications/native/NotificationsList/Divider/index.tsx (1)
1-24: Clean implementation of the "Read" divider component.The divider component is implemented as a simple, reusable component that creates a visual separator with text. It follows best practices by using the theme provider for consistent styling and separating styles into their own file.
packages/components/modules/notifications/native/NotificationsList/index.tsx (5)
18-18: Import of the Divider component looks good.This import is necessary for the new feature that divides notifications into read and unread sections.
52-52: Change in fetch policy may improve UI responsiveness.Changing from 'network-only' to 'store-and-network' will allow the component to display cached data immediately while fetching fresh data in the background. This should provide a better user experience by reducing perceived loading times.
80-83: FlatList update properly passes index parameter.The update to pass both item and index to renderNotificationItem is necessary for the divider logic to work correctly.
65-70: NotificationItem now receives refetch callback for dynamic updates.Adding the refetch prop allows notifications to be dynamically moved to the read group when marked as read, satisfying the requirement to update the list without page redirects.
57-60: Consider adding a test for the edge case of all read notifications.The current implementation will correctly hide the divider when all notifications are marked as read (since there won't be a transition from unread to read). However, it might be worth adding a test to explicitly verify this behavior.
Can you verify that when all notifications are marked as read, the divider is not displayed in any scenario?
anicioalexandre
added
needs changes
packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationsList/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationsList/index.tsx
Outdated
Show resolved
Hide resolved
tsl-ps2
force-pushed
the
BA-2392-divider-package
branch
from
563c4ba to
a566e9d
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
7-7: Notification Divider Feature EntryThe bullet point
- Native: Dividing notifications into unread/readaccurately reflects the new functionality for organizing notifications. Consider, if appropriate, adding a brief note regarding the dynamic behavior (e.g., auto-updating when notifications are marked as read) or the reverse chronological sorting to provide more context for users reviewing the changelog.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(2 hunks)packages/components/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx
🔇 Additional comments (2)
packages/components/CHANGELOG.md (2)
3-4: Ensure Consistent Version FormattingThe new release version
1.0.29is clearly added and follows the expected changelog structure. Verify that this bump aligns with your semantic versioning guidelines.
5-6: Changelog Section Header is ConsistentThe
"### Patch Changes"header is formatted consistently with previous entries. This ensures clarity in the type of update being released.
tsl-ps2
added
approved
tsl-ps2
force-pushed
the
BA-2392-divider-package
branch
from
8502d96 to
4b92372
Compare
|
Description
As a user, on the BaseApp React Native,I would like to see dividers in my notification list based on how old the notifications are, In order to distinguish between recent and older notifications.
Acceptance Criteria
Given I have multiple notifications, when I open the notifications page, then the notifications should be grouped by Read/Unread and separated by a divider.
The divider will have the "Read" Label
The reverse chronological sort should be maintained inside each group. (Most recent first)
Given a notification is mark as read, then it should go to the read group live.
If everything is read, then the divider should be hidden.
Approvd
https://app.approvd.io/silverlogic/BA/stories/38686
Summary by CodeRabbit
Dividercomponent to improve layout structure within the notifications list.