feat(addon-table): add optional first/last page buttons to the TablePagination component #6652
Conversation
pubiqq
requested review from
waterplea,
MarsiBarsi,
vladimirpotekhin,
nsbarsukov and
splincode
as code owners
Pull request was closed ✔️All saved screenshots (for current PR) were deleted 🗑️ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6652 +/- ##
==========================================
- Coverage 70.19% 63.52% -6.68%
==========================================
Files 1460 1087 -373
Lines 15934 11753 -4181
Branches 2292 1642 -650
==========================================
- Hits 11185 7466 -3719
+ Misses 4369 4102 -267
+ Partials 380 185 -195
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
BundleMonFiles updated (1)
Unchanged files (4)
Total files change +185B +0.03% Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
pubiqq
force-pushed
the
feat/table-pagination/first-last-page
branch
2 times, most recently
from
f533573
to
f6db09e
Compare
splincode
force-pushed
the
feat/table-pagination/first-last-page
branch
from
f6db09e
to
e0fdf2e
Compare
pubiqq
force-pushed
the
feat/table-pagination/first-last-page
branch
2 times, most recently
from
5a6d1da
to
1304414
Compare
pubiqq
force-pushed
the
feat/table-pagination/first-last-page
branch
2 times, most recently
from
5b5d487
to
826074a
Compare
pubiqq
force-pushed
the
feat/table-pagination/first-last-page
branch
from
826074a
to
822449c
Compare
…agination component
pubiqq
force-pushed
the
feat/table-pagination/first-last-page
branch
from
822449c
to
f27dac2
Compare
| const previousPage = this.previousPage; | ||
|
|
||
| if (previousPage === null) { | ||
| return; |
There was a problem hiding this comment.
Early returns make no sense if it's just one line. This looks like it would be better to just support null in selectPage and do the early return there.
| @@ -38,6 +38,9 @@ export class TuiTablePaginationComponent { | |||
| @Input() | |||
| public size = this.options.size; | |||
|
|
|||
| @Input() | |||
| public showFirstLastPageButtons = false; | |||
There was a problem hiding this comment.
Sorry for taking so long to review this. We need shorter names and we do not need to deprecate and rename things, let's do this with minimum intervention to how it works right now. If we only want it in 4.0, we can just rename everything to better suited names.
| return this.previousPage === null; | ||
| } | ||
|
|
||
| protected get previousPageDisabled(): boolean { |
There was a problem hiding this comment.
Let's drop Page from all these getters. This is a pagination component, it's implicit that we are talking about pages. And class prop names are not minified by compiler so it's better to keep them sort bundlesize-wise.
| } | ||
|
|
||
| protected get lastPageDisabled(): boolean { | ||
| return this.nextPage === null; |
There was a problem hiding this comment.
Why do we have 2 getters that do the same?
| /** | ||
| * Returns the index of the first page. | ||
| */ | ||
| private get firstPage(): number { |
There was a problem hiding this comment.
This getter makes no sense. It's obvious that first index is 0, there's no need to move into a semantic getter.
| /** | ||
| * Returns the index of the last page. | ||
| */ | ||
| private get lastPage(): number { |
There was a problem hiding this comment.
It is only used once, let's not bloat class with a getter for lastPage, just inline this.pageCount - 1 where you use it, it's self explanatory.
New property for the TablePagination component -
showFirstLastPageButtons:showFirstLastPageButtons = false(default)showFirstLastPageButtons = true