-
Notifications
You must be signed in to change notification settings - Fork 6
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
Kt 2021 11 pagination #3973
Kt 2021 11 pagination #3973
Conversation
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.
nice!!
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.
It looks good and works well! (And I think for testing and for now it would be good, to keep the firts state of the template?)
25836d3
to
1c34c26
Compare
Coverage reportTotal coverage
Show new covered files 🌑Coverage of new files
Show files with reduced coverage 🔻Reduced coverage
Report generated by 🧪jest coverage report action from 6139057 |
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.
Yeah, I like that all the logic is in the API now. But I think, that can be done with less code by using what the paginator already gives you.
https://www.django-rest-framework.org/api-guide/pagination/#custom-pagination-styles
meinberlin/apps/budgeting/api.py
Outdated
return Response({ | ||
'status': True, | ||
'results': data, | ||
'meta': { |
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.
Same here: do you need all of that? Could you use the stuff you have?
…settings.py creating better response structur from backend changing page_size to be set on backend only
24d27a8
to
b51a300
Compare
…t add what is really needed
b51a300
to
6139057
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.
Nice!
Some notes, that might help to understand what was intended :)
This react pagination should behave like the old one (rather than what is in the current design)
I am not sure if i utilized the pagination django part correctly, any suggestions appreciated.
You might expect it a bit different, but:
page_size = 0 won't show all, but 0 --> edge case (we should use rather a large number)