-
Notifications
You must be signed in to change notification settings - Fork 13k
chore: paginationData not being spread correctly in admin users
#37063
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
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughDestructures paginationData in UsersTable to pass explicit current/itemsPerPage and setter handlers to Pagination; story adds a mockedPagination object and supplies it to UsersTable via paginationData in the Storybook template. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UsersTable
participant Pagination
Note over UsersTable,Pagination: UsersTable now passes explicit pagination props and setters
User->>Pagination: click page / change itemsPerPage
Pagination-->>UsersTable: onSetCurrent(page) / onSetItemsPerPage(size)
rect rgba(220,235,255,0.6)
note right of UsersTable: call setCurrent(page) / setItemsPerPage(size)
UsersTable-->>UsersTable: update state and compute paginationProps
end
UsersTable-->>Pagination: render with { current, itemsPerPage, ...paginationProps }
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37063 +/- ##
===========================================
+ Coverage 67.35% 67.37% +0.01%
===========================================
Files 3326 3326
Lines 113195 113207 +12
Branches 20534 20534
===========================================
+ Hits 76240 76270 +30
+ Misses 34350 34331 -19
- Partials 2605 2606 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
c553308 to
9f03380
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (2)
10-17: Tighten typing and setter signatures formockedPagination.
- Add
satisfiesto lock the shape toUsersTable’spaginationDatatype.- Accept a number in the setter no‑ops to avoid potential
strictFunctionTypesfriction.Apply this diff:
-const mockedPagination = { +const mockedPagination = { current: 0, - setCurrent: () => undefined, + setCurrent: (_: number) => undefined, itemsPerPage: 25 as const, - setItemsPerPage: () => undefined, + setItemsPerPage: (_: number) => undefined, itemsPerPageLabel: () => 'Items per page:', showingResultsLabel: () => 'Showing results 1 - 5 of 5', -}; +} satisfies NonNullable<Parameters<typeof UsersTable>[0]['paginationData']>;
19-19: Don’t hardcodepaginationDatain the Template; pass via args.Keeps the story flexible and allows other stories/controls to override pagination.
Apply this diff:
-const Template: StoryFn<typeof UsersTable> = (args) => <UsersTable {...args} paginationData={mockedPagination} />; +const Template: StoryFn<typeof UsersTable> = (args) => <UsersTable {...args} />;Then set it in Default.args (outside the changed hunk):
Default.args = { // ...existing args paginationData: mockedPagination, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx(1 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTable.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/users/UsersTable/UsersTable.tsx
🔇 Additional comments (1)
apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
11-11: Confirm Pagination page index base
Verify that thePaginationcomponent from@rocket.chat/fuselageexpects a 1-basedcurrentpage index; if so, updatemockedPagination.currentto1instead of0.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
Documentation