BA-1584 Add Storybook for Notifications #128
Conversation
|
matheusysd
force-pushed
the
BA-1738-notifications-storybook
branch
from
29dbb18 to
2f60f5a
Compare
|
can we mock the "Mark all as read" action as well? |
There was a problem hiding this comment.
This first list is related to the design-system package stories, since both storybooks are composed here. So you don't need to add stories here, since they are from components package and if you can, please change the name storiesOrder to designSystemStoriesOrder. Besides that, you can move //Notifications prior to // Social and NotificationItem before NotificationsList
There was a problem hiding this comment.
look like body is not appearing on the story
There was a problem hiding this comment.
It seems we have some bug here, @anicioalexandre tried to help me, but we decided to left like this and work on a bug later
There was a problem hiding this comment.
i think we need to mock the data for the drawer as well, now is just loading
There was a problem hiding this comment.
we decided to mock only the first call, and I think that's why we are not rendering the list. Now that we have queueOperationResolver I'll try using it
matheusysd
force-pushed
the
BA-1738-notifications-storybook
branch
from
2996eb1 to
1796cfe
Compare
WalkthroughThe changes in this pull request introduce enhancements to the Storybook configuration for notifications components. Key modifications include the addition of new components, mock data, and Storybook stories for Changes
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/.storybook/preview.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 1796cfe3f86d70a0e39fd7e13452e4e7248b6db1 and 60e7d90. 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 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: 9
🧹 Outside diff range and nitpick comments (16)
packages/components/modules/notifications/NotificationsPopover/__storybook__/mockResolvers.ts (1)
1-3: Consider adding TypeScript interfaces for better type safety.The mock data structure looks good, but could benefit from explicit typing.
Consider adding an interface:
interface NotificationsResponse { data: { me: { id: string; notificationsUnreadCount: number; }; }; } export const unreadNotificationsMockData: NotificationsResponse = { data: { me: { id: 'user-1', notificationsUnreadCount: 2 } }, }packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/mockResolvers.ts (1)
1-28: Consider enhancing the mock data structureThe mock resolver provides a good foundation but could be enhanced for better testing coverage:
- The
data.extraobject is empty but might need realistic test data- Consider adding more fields that might be relevant for notifications (e.g.,
createdAt,updatedAt)- The actor object could include more fields like
nameoravatarthat might be needed in the UIHere's a suggested enhancement:
export const mockResolvers = { Node: () => ({ id: 'Tm90aWZpY2F0aW9uOjIyNQ==', unread: true, pk: 225, timestamp: '2024-10-25T19:46:09.288290+00:00', + createdAt: '2024-10-25T19:46:09.288290+00:00', + updatedAt: '2024-10-25T19:46:09.288290+00:00', level: 'INFO', verb: 'COMMENTS.COMMENT_REPLY_CREATED', description: 'Someone replied to your comment 1.', data: { - extra: {}, + extra: { + commentThread: 'thread-123', + contextUrl: '/posts/123#comment-237' + }, }, actor: { __typename: 'Profile', id: 'UHJvZmlsZToxNTM=', + name: 'John Doe', + avatar: 'https://example.com/avatar.jpg' }, target: { id: 'Q29tbWVudDoyMzc=', __typename: 'Comment', + body: 'Original comment text' }, actionObject: { id: 'Q29tbWVudDoyNDg=', __typename: 'Comment', body: 'This is the first comment reply.', }, __typename: 'Notification', }), }packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/NotificationItemWithQuery/index.tsx (2)
8-19: Consider making the test ID configurable and documenting query options.While the implementation is functional, consider these improvements:
- Make the "test-id" configurable through props to support different test scenarios
- Document the purpose of the empty options object in useLazyLoadQuery
Here's a suggested improvement:
-const NotificationItemWithQuery = (props: NotificationItemProps) => { +interface NotificationItemWithQueryProps extends NotificationItemProps { + testId?: string; // Allow custom test IDs +} + +const NotificationItemWithQuery = ({ testId = 'test-id', ...props }: NotificationItemWithQueryProps) => { const data = useLazyLoadQuery<Query>( graphql` query NotificationItemWithQuery @relay_test_operation { - target: node(id: "test-id") { + target: node(id: $testId) { ...NotificationItemFragment } } `, - {}, + {}, // Empty options object - no variables needed for Storybook testing )
20-23: Consider adding runtime type validation.The type casting of
data.targetcould benefit from runtime validation to ensure type safety.Here's a suggested improvement:
+ if (!data.target) { + throw new Error('Failed to load notification data'); + } + return <NotificationItem {...props} notification={data.target as NotificationItemFragment$key} />packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/stories.tsx (1)
12-20: Enhance notification prop documentationWhile the basic documentation is present, consider enhancing the notification prop documentation:
- Add examples of valid notification objects
- Document required vs optional fields
- Include possible values or constraints
notification: { description: 'The notification data.', + description: 'The notification data object containing id, message, timestamp, and read status.', control: 'object', table: { type: { summary: 'NotificationItem_notification$key', + detail: `{ + id: string; + message: string; + timestamp: string; + read: boolean; + // Add other relevant fields + }` }, }, },packages/components/.storybook/decorators/withProviders.tsx (1)
28-28: Simplify boolean conditionRemove the redundant double negation.
- if (!!mockResolvers) { + if (mockResolvers) {🧰 Tools
🪛 Biome
[error] 28-28: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
packages/components/modules/notifications/NotificationsList/__storybook__/stories.tsx (3)
6-57: Add "Mark all as read" action to Storybook controlsBased on the PR comments, consider adding the "Mark all as read" action to allow interaction testing in Storybook.
const meta: Meta<typeof NotificationsList> = { title: '@baseapp-frontend | components/Notifications/NotificationsList', component: NotificationsList, tags: ['autodocs'], argTypes: { + onMarkAllAsRead: { + action: 'marked all as read', + description: 'Callback fired when marking all notifications as read', + table: { + type: { + summary: '() => void', + }, + }, + }, // ... existing argTypes }, }
11-55: Enhance prop descriptions for better documentationConsider adding more detailed descriptions for the component props:
- Include type constraints or accepted values
- Mention default behaviors
- Add examples where applicable
For instance:
setIsDrawerOpened: { control: false, - description: 'Function to toggle the Drawer open/close state.', + description: 'Function to toggle the Drawer open/close state. Called with the new state value when the drawer visibility changes.', // ... }, LoadingStateProps: { control: 'object', - description: 'Props to pass to the LoadingState component.', + description: 'Props to pass to the LoadingState component. Customize the loading state appearance (e.g., number of skeleton items to show).', // ... },
63-83: Consider adding error state and improving story namesThe stories could be enhanced with:
- An additional story for error states
- More descriptive names that indicate the use case
export const DefaultNotificationsList: Story = { - name: 'Default NotificationsList', + name: 'With Active Notifications', // ... } export const EmptyNotificationsList: Story = { - name: 'Empty NotificationsList', + name: 'With No Notifications', // ... } export const LoadingNotificationsList: Story = { - name: 'Loading NotificationsList', + name: 'In Loading State', // ... } +export const ErrorNotificationsList: Story = { + name: 'With Error State', + args: {}, + parameters: { + mockData: { + error: new Error('Failed to fetch notifications'), + data: null + }, + }, +}packages/components/modules/notifications/NotificationsPopover/__storybook__/stories.tsx (1)
6-84: Enhance props documentation with examplesThe meta configuration is well-structured, but the documentation could be more helpful by including:
- Example values for object-type props (DrawerProps, BadgeProps, etc.)
- Specific use cases or constraints for each component override
Consider enhancing the documentation like this:
DrawerProps: { control: 'object', description: 'Props to pass to the Drawer component.', table: { type: { summary: 'Partial<DrawerProps>', + detail: 'Example: { open: boolean, onClose: () => void, anchor: "right" }' }, }, },packages/components/.storybook/preview.ts (3)
Line range hint
37-112: Reduce code duplication between story order arraysThe
designSystemStoriesOrderandcomponentsStoriesOrderarrays contain identical entries up to the "General" section. Consider extracting these common entries to a shared array to improve maintainability.+ const commonStoriesOrder = [ + 'Iconography', + // Avatars + 'AvatarWithPlaceholder', + 'ClickableAvatar', + // ... other common entries + 'Logo', + 'Scrollbar', + ] - const designSystemStoriesOrder = [ - 'Iconography', - // Avatars - 'AvatarWithPlaceholder', - // ... duplicate entries - ] + const designSystemStoriesOrder = [...commonStoriesOrder] - const componentsStoriesOrder = [ - 'Iconography', - // Avatars - 'AvatarWithPlaceholder', - // ... duplicate entries - ] + const componentsStoriesOrder = [ + ...commonStoriesOrder, + // Navigation + 'NavigationLayout', + // ... remaining unique entries + ]
98-101: Reorder notification components for better logical flowThe notification components should follow a hierarchical order from smallest to largest component for better readability and understanding of the component structure.
//Notifications 'NotificationItem', - 'NotificationsList', - 'NotificationsPopover', + 'NotificationsPopover', + 'NotificationsList',
36-36: Enhance comment about external variablesThe current comment could be more descriptive about why external variables aren't accepted and provide context about the Storybook limitation.
- // NOTE: Storybook does not accept importing external variables for storySort, + // NOTE: Storybook's storySort function runs in an isolated context and cannot access + // external variables or imports. This is a known limitation, so we define the sort + // order arrays inline. See: https://storybook.js.org/docs/react/writing-stories/sorting-storiespackages/components/CHANGELOG.md (1)
3-8: Enhance the changelog entry with more detailed information.While the entry follows the correct format, consider expanding it to provide more comprehensive details about the changes, such as:
## 0.0.15 ### Patch Changes -Add Storybook for notifications components. +Add Storybook for notifications components: +- Add stories for NotificationsPopover, NotificationsList, and NotificationItem components +- Include various states: notifications list with items, loading state, and empty state +- Add mock data and resolvers for testing notification interactionsThis would better reflect the scope of changes mentioned in the PR objectives and provide more valuable information for future reference.
packages/components/modules/notifications/NotificationsList/__storybook__/mockResolvers.ts (2)
17-322: Add mock resolver for "Mark all as read" actionIn response to @deboracosilveira's comment about mocking the "Mark all as read" action, consider adding a mock resolver that:
- Updates the unread status of all notifications
- Resets the unreadCount to 0
- Triggers a UI refresh
Would you like me to help implement the mock resolver for the "Mark all as read" action?
1-322: Add TypeScript interfaces for mock data structureConsider adding TypeScript interfaces to define the shape of the mock data. This would:
- Provide better type safety
- Make the data structure more maintainable
- Help catch potential structure mismatches early
- Serve as documentation for other developers
interface NotificationNode { id: string; unread: boolean; pk: number; timestamp: string; level: 'INFO' | 'WARNING' | 'ERROR'; verb: string; description: string; data: { extra: Record<string, unknown>; }; actor: { __typename: string; id: string; }; target: { id: string; __typename: string; }; actionObject: { id: string; __typename: string; body: string; }; __typename: 'Notification'; } interface NotificationsData { data: { me: { id: string; notificationsUnreadCount: number; notifications: { edges: Array<{ cursor: string; node: NotificationNode; }>; pageInfo: { hasNextPage: boolean; endCursor: string | null; }; }; }; }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between bd53fc7 and 1796cfe3f86d70a0e39fd7e13452e4e7248b6db1.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/NotificationItemWithQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (10)
packages/components/.storybook/decorators/withProviders.tsx(2 hunks)packages/components/.storybook/preview.ts(3 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/NotificationItemWithQuery/index.tsx(1 hunks)packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/mockResolvers.ts(1 hunks)packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/stories.tsx(1 hunks)packages/components/modules/notifications/NotificationsList/__storybook__/mockResolvers.ts(1 hunks)packages/components/modules/notifications/NotificationsList/__storybook__/stories.tsx(1 hunks)packages/components/modules/notifications/NotificationsPopover/__storybook__/mockResolvers.ts(1 hunks)packages/components/modules/notifications/NotificationsPopover/__storybook__/stories.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
packages/components/.storybook/decorators/withProviders.tsx
[error] 28-28: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (6)
packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/mockResolvers.ts (2)
24-24: Address the body visibility issue in Storybook
A previous review comment indicated that the body field wasn't appearing in the story. Let's verify if this field is being properly handled in the component and story.
#!/bin/bash
# Description: Check how the body field is used in the component and story files
echo "Checking for body field usage in component and story files..."
rg -A 5 "body.*prop|body.*interface" packages/components/modules/notifications
echo "Checking for body field rendering in stories..."
rg -A 5 "body.*render|body.*{.*}" packages/components/modules/notifications3-3: Verify the GraphQL ID encoding format
The IDs appear to be base64 encoded, which is common in GraphQL. Let's verify they decode to meaningful values:
Also applies to: 15-15, 18-18, 22-22
packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/NotificationItemWithQuery/index.tsx (2)
1-7: LGTM! Well-structured imports with proper type definitions.
The imports are well-organized and make good use of generated types for type safety.
1-23: Verify Storybook integration and mock data setup.
Let's verify the component is properly integrated with Storybook configuration and mock data.
packages/components/modules/notifications/NotificationsList/NotificationItem/__storybook__/stories.tsx (1)
3-5: Verify component usage consistency
The story configuration uses NotificationItemWithQuery as the main component in meta while using NotificationItem for the Story type. This might lead to confusion. Please verify if this is intentional and consider:
- Using consistent component types
- Adding documentation to explain why different components are used
packages/components/.storybook/decorators/withProviders.tsx (1)
27-34: LGTM! Well-structured mock data handling
The useEffect implementation is clean and logical:
- Sets up mock resolvers if provided
- Resolves operations with combined mock data
- Correctly specifies all dependencies
This implementation aligns well with the PR objective of supporting Storybook for Notifications components.
🧰 Tools
🪛 Biome
[error] 28-28: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test scenarios and "Mark all as read" action.
While the empty state is covered, consider adding:
- Error state scenarios
- Mock resolver for "Mark all as read" action (as requested in PR comments)
- Edge cases (e.g., very high notification counts)
Here's a suggested implementation:
export const markAllAsReadMockResolver = {
Mutation: {
markAllNotificationsAsRead: () => ({
success: true,
notifications: [],
}),
},
}
export const errorStateMockData = {
error: {
message: 'Failed to fetch notifications',
},
}
export const highCountMockData = {
data: { me: { id: 'user-1', notificationsUnreadCount: 99 } },
}There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more story variants and interactions
Consider enhancing the stories with:
- Different notification states (read/unread)
- Various content lengths
- Interactive examples (e.g., marking as read)
- Error states
This aligns with the PR comment about mocking "Mark all as read" functionality.
export const DefaultNotificationItem: Story = {
name: 'Default NotificationItem',
parameters: {
mockResolvers,
},
}
+
+export const ReadNotification: Story = {
+ name: 'Read Notification',
+ parameters: {
+ mockResolvers: {
+ ...mockResolvers,
+ NotificationItem: () => ({
+ ...mockResolvers.NotificationItem(),
+ read: true,
+ }),
+ },
+ },
+}
+
+export const LongContent: Story = {
+ name: 'Long Content Notification',
+ parameters: {
+ mockResolvers: {
+ ...mockResolvers,
+ NotificationItem: () => ({
+ ...mockResolvers.NotificationItem(),
+ message: 'This is a very long notification message that demonstrates how the component handles text wrapping and overflow...',
+ }),
+ },
+ },
+}
+
+export const WithInteraction: Story = {
+ name: 'Interactive Notification',
+ parameters: {
+ mockResolvers,
+ },
+ play: async ({ canvasElement }) => {
+ // Add interaction test here
+ },
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
💡 Codebase verification
Change to mockResolvers affects existing stories and tests
Multiple Storybook story and test files use mockResolvers. Please ensure that changing the default value to undefined does not break these existing implementations.
🔗 Analysis chain
Verify backward compatibility of mockResolvers change
Changing the default value from an empty object to undefined might affect existing stories that expect mockResolvers to be an object. Please ensure this change doesn't break existing stories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for stories that might be affected by this change
rg -l "mockResolvers" --type stories
# Check for any direct references to mockResolvers in test files
rg -l "mockResolvers" --type test
Length of output: 131
Script:
#!/bin/bash
# Search for Storybook stories that might be affected by this change
rg -l "mockResolvers" --glob "*.stories.tsx" --glob "*.stories.jsx"
# Search for test files that reference mockResolvers
rg -l "mockResolvers" --glob "*.test.tsx" --glob "*.spec.tsx" --glob "*.test.jsx" --glob "*.spec.jsx"
Length of output: 318
Script:
#!/bin/bash
# Search for Storybook stories that might be affected by this change
fd --glob "*.stories.tsx" --glob "*.stories.jsx" -e tsx -e jsx | xargs rg -l "mockResolvers"
# Search for test files that reference mockResolvers
fd --glob "*.test.tsx" --glob "*.spec.tsx" --glob "*.test.jsx" --glob "*.spec.jsx" -e tsx -e jsx | xargs rg -l "mockResolvers"
Length of output: 3854
Script:
#!/bin/bash
# Search for Storybook stories that might be affected by this change
fd --glob "*.stories.tsx" --glob "*.stories.jsx" | xargs rg -l "mockResolvers"
# Search for test files that reference mockResolvers
fd --glob "*.test.tsx" --glob "*.spec.tsx" --glob "*.test.jsx" --glob "*.spec.jsx" | xargs rg -l "mockResolvers"
Length of output: 3826
There was a problem hiding this comment.
Add mock data for LoadingNotificationsList story
The LoadingNotificationsList story is missing mock data in its parameters. Consider adding appropriate loading state mock data to simulate the loading experience accurately.
export const LoadingNotificationsList: Story = {
name: 'Loading NotificationsList',
args: {},
- parameters: {},
+ parameters: {
+ mockData: {
+ loading: true,
+ data: null
+ },
+ },
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const LoadingNotificationsList: Story = { | |
| name: 'Loading NotificationsList', | |
| args: {}, | |
| parameters: {}, | |
| } | |
| export const LoadingNotificationsList: Story = { | |
| name: 'Loading NotificationsList', | |
| args: {}, | |
| parameters: { | |
| mockData: { | |
| loading: true, | |
| data: null | |
| }, | |
| }, | |
| } |
There was a problem hiding this comment.
💡 Codebase verification
Mock data for NotificationsList is not properly implemented in Storybook
The mockResolvers.ts in the Storybook directory does not define notificationsListMockData or emptyNotificationsListMockData. Please ensure that these mock data are correctly implemented and imported within the Storybook stories.
🔗 Analysis chain
Verify mock data implementation for notifications
The mock data imports look correct, but let's verify the implementation of the mock resolvers to ensure they cover all necessary notification states and properties.
Correct the search command to target TypeScript files appropriately
The previous rg command failed due to an unrecognized file type. Please verify mock data implementations using the updated shell script below.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mock resolver implementation
echo "Searching for mockResolvers.ts file..."
fd mockResolvers.ts
# Check mock data structure
echo "Checking mock data structure..."
rg -A 10 "notificationsListMockData|emptyNotificationsListMockData" --type typescript
Length of output: 1107
Script:
#!/bin/bash
# Search for mock resolver implementation files
echo "Searching for mockResolvers.ts files..."
fd mockResolvers.ts
# Check mock data structure
echo "Checking mock data structure..."
rg -A 10 "notificationsListMockData|emptyNotificationsListMockData" -g "*.{ts,tsx}"
Length of output: 16823
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more comprehensive story variations
The current stories are missing several key scenarios mentioned in the PR objectives:
- Loading state story
- Interactive "Mark all as read" functionality
- Various drawer states and interactions
Consider adding these additional stories:
export const LoadingState: Story = {
name: 'Loading State',
args: {},
parameters: {
mockData: {
loading: true
}
}
}
export const WithMarkAllAsRead: Story = {
name: 'With Mark All as Read Action',
args: {},
parameters: {
mockData: {
...unreadNotificationsMockData,
resolvers: {
Mutation: {
markAllNotificationsAsRead: () => ({
success: true
})
}
}
}
}
}There was a problem hiding this comment.
💡 Codebase verification
Missing mock implementation for "Mark all as read" action
The markAllAsRead mock resolver is not found in the codebase. Please implement the required mock to align with PR objectives.
🔗 Analysis chain
Verify mock data implementation for complete notification scenarios
Based on the PR objectives and past review comments, we should ensure the mock data covers all notification scenarios, including the "Mark all as read" action.
Let's check the mock resolvers implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check mock resolvers implementation
rg -A 10 "unreadNotificationsMockData|unreadNotificationsEmptyMockData" --type typescript
Length of output: 126
Script:
#!/bin/bash
# Check mock resolvers implementation in TypeScript and TSX files
rg -A 10 "unreadNotificationsMockData|unreadNotificationsEmptyMockData" --type ts --type tsx
Length of output: 122
Script:
#!/bin/bash
# Check mock resolvers implementation in TypeScript and TSX files
rg -A 10 "unreadNotificationsMockData|unreadNotificationsEmptyMockData" -g "*.ts" -g "*.tsx"
Length of output: 16559
Script:
#!/bin/bash
# Check for "markAllAsRead" mock resolver implementation in TypeScript and TSX files
rg "markAllAsRead" -g "*.ts" -g "*.tsx"
Length of output: 39
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance mock data variety for better test coverage
The current mock data has several patterns that might limit testing scenarios:
- All notifications are unread
- All notifications are comment replies
- All notifications target the same comment
- All notifications have the same actor
Consider diversifying the mock data to:
- Include both read and unread notifications
- Add different notification types (not just comment replies)
- Vary the target and actor IDs
- Include different notification levels (not just INFO)
This would help ensure the component handles various real-world scenarios correctly.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adjusting unread count in empty state mock
There's an inconsistency in the empty state mock where notificationsUnreadCount is 12 but the notifications list is empty. This might not accurately represent a real empty state scenario and could confuse developers. Consider setting notificationsUnreadCount to 0 to maintain data consistency.
me: {
id: 'user-1',
- notificationsUnreadCount: 12,
+ notificationsUnreadCount: 0,
notifications: {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const emptyNotificationsListMockData = { | |
| data: { | |
| me: { | |
| id: 'user-1', | |
| notificationsUnreadCount: 12, | |
| notifications: { | |
| edges: [], | |
| pageInfo: { | |
| hasNextPage: false, | |
| endCursor: null, | |
| }, | |
| }, | |
| }, | |
| }, | |
| } | |
| export const emptyNotificationsListMockData = { | |
| data: { | |
| me: { | |
| id: 'user-1', | |
| notificationsUnreadCount: 0, | |
| notifications: { | |
| edges: [], | |
| pageInfo: { | |
| hasNextPage: false, | |
| endCursor: null, | |
| }, | |
| }, | |
| }, | |
| }, | |
| } |
matheusysd
force-pushed
the
BA-1738-notifications-storybook
branch
from
1796cfe to
60e7d90
Compare
|
Summary by CodeRabbit
Release Notes
New Features
NotificationItemWithQueryandNotificationsPopoverwith respective stories.Bug Fixes
withProvidersfunction.Documentation