-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update pagination logic in list pages #1
Conversation
WalkthroughThe changes introduce a new method, Changes
Poem
Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. 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 (
|
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: 1
Outside diff range, codebase verification and nitpick comments (1)
src/Resources/CountryResource/Pages/ListCountries.php (1)
29-32
: LGTM!The method correctly implements the pagination logic based on the PR objectives and the AI-generated summary.
Consider using the
paginate
method for more advanced pagination scenarios.The method uses the
simplePaginate
method, which is suitable for simple pagination scenarios. However, for more advanced pagination scenarios, thepaginate
method can be used instead.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/Resources/CountryResource/Pages/ListCountries.php (2 hunks)
- src/Resources/CurrencyResource/Pages/ListCurrencies.php (2 hunks)
- src/Resources/LanguageResource/Pages/ListLanguages.php (2 hunks)
- src/Resources/LocationResource/Pages/ListLocations.php (2 hunks)
Additional comments not posted (6)
src/Resources/LocationResource/Pages/ListLocations.php (2)
9-10
: LGTM!The new imports are necessary for the
paginateTableQuery
method.
29-32
: LGTM!The
paginateTableQuery
method is correctly implemented and matches the PR objectives. It usessimplePaginate
for efficient pagination and correctly handles the case when the number of records per page is set to 'all'.src/Resources/CountryResource/Pages/ListCountries.php (1)
9-10
: LGTM!The imports are necessary for the new
paginateTableQuery
method.src/Resources/CurrencyResource/Pages/ListCurrencies.php (2)
10-11
: LGTM!The code changes are approved.
30-33
: LGTM!The code changes are approved.
src/Resources/LanguageResource/Pages/ListLanguages.php (1)
10-11
: LGTM!The imports are necessary for the new
paginateTableQuery
method.
protected function paginateTableQuery(Builder $query): Paginator | ||
{ | ||
return $query->simplePaginate(($this->getTableRecordsPerPage() === 'all') ? $query->count() : $this->getTableRecordsPerPage()); | ||
} |
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.
Use the null coalescing operator ??
for a more concise implementation.
The method can be further improved by using the null coalescing operator ??
instead of the ternary operator.
Apply this diff to refactor the method:
-protected function paginateTableQuery(Builder $query): Paginator
-{
- return $query->simplePaginate(($this->getTableRecordsPerPage() === 'all') ? $query->count() : $this->getTableRecordsPerPage());
-}
+protected function paginateTableQuery(Builder $query): Paginator
+{
+ return $query->simplePaginate($this->getTableRecordsPerPage() === 'all' ? $query->count() : $this->getTableRecordsPerPage() ?? 10);
+}
Committable suggestion was skipped due to low confidence.
This pull request updates the pagination logic in the list pages for countries, currencies, languages, and locations. The
paginateTableQuery
method in each page now returns aPaginator
instance instead of aLengthAwarePaginator
instance. This change ensures that the pagination works correctly when the number of records per page is set to 'all'.Also keeps the page load faster because not all data are loaded before opening the page. Instead the page loads a specific amount of data so when a user click on next button, the next N data will be loaded from the database.
Summary by CodeRabbit
These improvements provide a more flexible and user-friendly experience when managing and viewing records in the application.