BA 2373 add Infinit scroller to notifications list and design system #240
Conversation
Ronan-Fernandes
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis PR introduces a new constant, 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/common/constants.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/notifications/common/types.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/design-system/components/native/views/InfiniteScrollerView/styles.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/design-system/.eslintrc.js
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (3)
packages/design-system/components/native/views/InfiniteScrollerView/types.ts (1)
13-15: Consider simplifying the InfiniteScrollerViewProps type definition.The type definition has a redundant intersection with
FlashListProps<TItem>appearing twice, as it's already included in line 13 and then partially omitted in line 14. This could lead to confusion for developers.-export type InfiniteScrollerViewProps<TItem> = FlashListProps<TItem> & - Omit<FlashListProps<TItem>, 'ListFooterComponent'> & +export type InfiniteScrollerViewProps<TItem> = Omit<FlashListProps<TItem>, 'ListFooterComponent'> & (ListFooterComponent<TItem> | IsLoading)packages/design-system/components/native/views/InfiniteScrollerView/index.tsx (1)
68-72: Consider a more explicit empty state for non-loading scenarios.The current implementation returns a simple padded View when
isLoadingis false. You might want to consider a more explicit approach or remove the padding entirely when there are no more items to load.- const renderFooterLoadingState = () => { - if (!isLoading) return <View style={{ paddingTop: 24 }} /> - - return <LoadingScreen size="small" /> - } + const renderFooterLoadingState = () => { + if (!isLoading) return null + + return <LoadingScreen size="small" /> + }packages/components/modules/notifications/native/NotificationsList/index.tsx (1)
40-44: Remove outdated TODO comment.The TODO comment "handle infinite scroll" is now obsolete since the implementation already addresses this with the InfiniteScrollerView and pagination hooks.
- // TODO: handle infinite scroll const { data, loadNext, isLoadingNext, hasNext, refetch } = usePaginationFragment< NotificationsListQueryType, NotificationsListFragment$key >(NotificationsListFragment, me)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/components/modules/notifications/common/constants.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(3 hunks)packages/components/modules/notifications/web/NotificationsList/constants.ts(0 hunks)packages/components/modules/notifications/web/NotificationsList/index.tsx(1 hunks)packages/design-system/components/native/views/InfiniteScrollerView/index.tsx(1 hunks)packages/design-system/components/native/views/InfiniteScrollerView/styles.ts(1 hunks)packages/design-system/components/native/views/InfiniteScrollerView/types.ts(1 hunks)packages/design-system/components/native/views/index.ts(1 hunks)packages/design-system/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/notifications/web/NotificationsList/constants.ts
🔇 Additional comments (11)
packages/components/modules/notifications/common/constants.ts (1)
9-9: Good addition of a pagination constantAdding this constant to the common module is a good practice for reusability across both web and native implementations. The value of 5 notifications seems reasonable for batch loading.
packages/design-system/package.json (1)
72-72:✅ Verification successful
FlashList dependency added appropriately
The addition of
@shopify/flash-listis appropriate for implementing the infinite scroller functionality. FlashList is a high-performance alternative to FlatList that offers better memory usage and rendering speed.Verify React Native compatibility with this version of FlashList:
🏁 Script executed:
#!/bin/bash # Check React Native version in the project and FlashList compatibility echo "Checking React Native version in package.json..." cat package.json | grep "\"react-native\":" || echo "Not found in root package.json" echo "Checking in workspace packages..." find packages -name "package.json" -exec grep -l "\"react-native\":" {} \; -exec grep "\"react-native\":" {} \;Length of output: 656
FlashList dependency addition confirmed—React Native version appears compatible
The script output confirms that all workspace packages (including design-system) specify React Native using the alias
"catalog:react-native-core". This indicates that the project is consistently using a catalog-based version of React Native, which should be compatible with@shopify/flash-list@^1.7.5as long as the catalog mapping meets FlashList’s requirements.
- Location:
packages/design-system/package.json(line 72) shows the added dependency.- Verification: All relevant packages use
"react-native": "catalog:react-native-core", suggesting consistent configuration across the workspace.No further modifications are needed based on the current verification.
packages/design-system/components/native/views/InfiniteScrollerView/styles.ts (1)
1-7: Appropriate styling for infinite scrollerThe style implementation is clean and minimal. Setting the container height to 100% ensures the component fills its parent container, which is necessary for proper scrolling functionality.
packages/components/modules/notifications/web/NotificationsList/index.tsx (2)
15-15: Good refactoring of constant importImporting the constant from the common module instead of a local constants file is a good practice for code organization and reuse.
99-100: Consider addressing the overscan TODO commentThere's a commented TODO regarding potential "Maximum call stack size exceeded" errors when using overscan with the
NUMBER_OF_NOTIFICATIONS_TO_LOAD_NEXTvalue. This might be worth investigating further to improve scrolling performance without causing stack overflow issues.Could you provide more context on the Maximum call stack size exceeded error mentioned in the TODO comment? This might indicate a need for an alternative approach to improve scrolling performance without running into stack limitations.
packages/design-system/components/native/views/index.ts (1)
3-4: Properly exports new InfiniteScrollerView component and types.The exports follow the established pattern in the design system, maintaining consistency with how other components are exported.
packages/design-system/components/native/views/InfiniteScrollerView/types.ts (1)
3-11: Well-structured mutual exclusivity pattern for component props.The type definitions effectively create mutual exclusivity between
ListFooterComponentandisLoadingprops, ensuring clear API usage patterns and preventing conflicting prop combinations.packages/design-system/components/native/views/InfiniteScrollerView/index.tsx (2)
10-59: Excellent JSDoc documentation.The comprehensive documentation with clear descriptions and usage examples will help developers understand how to use this component correctly.
62-83: Well-structured component with proper prop handling.The component correctly handles the props and provides a good implementation of an infinite scroller using FlashList. The conditional rendering of the footer loading state is a good approach.
packages/components/modules/notifications/native/NotificationsList/index.tsx (2)
5-5: Properly imports InfiniteScrollerView.The import statement has been updated to include the new InfiniteScrollerView component from the design system.
73-88: Excellent implementation of infinite scrolling for notifications.The InfiniteScrollerView is well-integrated with proper configuration for data loading, rendering, and pagination. The component correctly uses the pagination fragment hooks (loadNext, isLoadingNext, hasNext) to manage the infinite scrolling behavior.
packages/design-system/components/native/views/InfiniteScrollerView/index.tsx
Outdated
Show resolved
Hide resolved
Ronan-Fernandes
changed the title
anicioalexandre
added
needs changes
packages/components/modules/notifications/native/NotificationsList/index.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/components/native/views/InfiniteScrollerView/index.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/components/native/views/InfiniteScrollerView/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationsList/index.tsx
Outdated
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2373-FE-Notifications-infinite-scroller
branch
from
2c7d261 to
a576100
Compare
Ronan-Fernandes
force-pushed
the
BA-2373-FE-Notifications-infinite-scroller
branch
from
a576100 to
b1e241b
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/modules/notifications/native/NotificationsList/styles.ts (1)
20-23: Consider adding a comment explaining the magic number or using a named constant.The
paddingBottom: 74uses a specific value that might not be immediately clear to other developers. To improve code maintainability, consider:
- Adding a comment explaining what this padding accommodates (e.g., bottom navigation bar, toolbar, etc.)
- Or extracting this value to a named constant to make its purpose obvious
Additionally, since both
containerandlistContainerhaveheight: '100%', ensure this duplication is intentional and necessary for your nested component structure.+ // Named constant for bottom padding to account for navigation bar + const BOTTOM_NAV_HEIGHT = 74; StyleSheet.create({ // ... listContainer: { height: '100%', - paddingBottom: 74, + paddingBottom: BOTTOM_NAV_HEIGHT, }, })packages/design-system/components/native/views/InfiniteScrollerView/index.tsx (1)
7-7: Make sure isLoading is correctly typed in the component props.The JSDoc states that
isLoadingis required whenListFooterComponentis not provided, but there's no runtime validation to enforce this requirement. Consider adding a runtime check to ensure this constraint is met.const InfiniteScrollerView = <TItem,>({ isLoading, estimatedItemSize = 200, ListFooterComponent, ...props }: InfiniteScrollerViewProps<TItem>) => { + // Validate required props based on conditions + if (!ListFooterComponent && isLoading === undefined) { + console.warn('isLoading is required when ListFooterComponent is not provided'); + } const renderFooterLoadingState = () => { if (!isLoading) return <View style={{ paddingTop: 24 }} /> return <LoadingScreen size="small" /> }packages/design-system/CHANGELOG.md (1)
7-7: Fix typo in CHANGELOG entry.There's a typo in "infinit" which should be "infinite".
-Add infinit scroller custom component based on @shopify/flash-list 1.7.6 +Add infinite scroller custom component based on @shopify/flash-list 1.7.6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/notifications/common/constants.ts(1 hunks)packages/components/modules/notifications/common/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(4 hunks)packages/components/modules/notifications/native/NotificationsList/styles.ts(1 hunks)packages/components/modules/notifications/web/NotificationsList/constants.ts(0 hunks)packages/components/modules/notifications/web/NotificationsList/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/native/views/InfiniteScrollerView/index.tsx(1 hunks)packages/design-system/components/native/views/InfiniteScrollerView/styles.ts(1 hunks)packages/design-system/components/native/views/InfiniteScrollerView/types.ts(1 hunks)packages/design-system/components/native/views/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/notifications/web/NotificationsList/constants.ts
✅ Files skipped from review due to trivial changes (2)
- packages/components/package.json
- packages/wagtail/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/components/modules/notifications/common/constants.ts
- packages/components/modules/notifications/web/NotificationsList/index.tsx
- packages/design-system/components/native/views/index.ts
- packages/design-system/components/native/views/InfiniteScrollerView/types.ts
- packages/design-system/components/native/views/InfiniteScrollerView/styles.ts
- packages/components/modules/notifications/native/NotificationsList/index.tsx
- packages/design-system/package.json
🧰 Additional context used
🧬 Code Definitions (1)
packages/design-system/components/native/views/InfiniteScrollerView/index.tsx (2)
packages/design-system/components/native/views/InfiniteScrollerView/types.ts (1)
InfiniteScrollerViewProps(13-15)packages/design-system/components/native/views/InfiniteScrollerView/styles.ts (1)
styles(3-7)
🔇 Additional comments (5)
packages/components/modules/notifications/common/types.ts (2)
1-2: LGTM: Clean and proper import statement.The import statement correctly imports the GraphQL fragment data type from the generated file, which will be used to derive the notification types.
3-5: Well-structured type definitions that follow best practices.These type definitions follow TypeScript best practices by:
- Using
NonNullableto ensure type safety- Creating a logical hierarchical structure that maps to GraphQL's cursor-based pagination pattern
- Properly typing array elements with
[number]index accessThese types will provide proper type safety when working with notification data in the new infinite scrolling implementation.
packages/design-system/components/native/views/InfiniteScrollerView/index.tsx (1)
1-82: Well-structured component with comprehensive documentation.The
InfiniteScrollerViewcomponent is thoughtfully implemented with clear documentation, proper type handling, and good examples. The integration with Shopify's FlashList and the conditional footer rendering make it a flexible solution for infinite scrolling lists.A few observations:
- The component correctly implements generic type handling as suggested in previous reviews
- The loading state handling is clean and straightforward
- The JSDoc is thorough and provides clear usage examples
packages/components/CHANGELOG.md (1)
3-9: LGTM - Changelog correctly documents the changes.The CHANGELOG.md entry properly documents the addition of the infinite scroller functionality to RN notifications list and the dependency update to the design system.
packages/wagtail/CHANGELOG.md (1)
3-8: LGTM - Changelog correctly documents the dependency update.The CHANGELOG.md entry properly documents the dependency update to the design system package.
__package_name__package update -v __package_version__Summary by CodeRabbit
New Features
Refactor