Harmonize CatalogTable
#28449
Harmonize CatalogTable
#28449
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
- Show pagination text for `OffsetPagination` - Use same `OffsetPaginatedCatalogTable` also as fallback if no pagination is set - Do not show paging if there is only one page Signed-off-by: Andreas Berger <andreas@berger-ecommerce.com>
Andy2003
force-pushed
the
bugfix/harmonize-catalog-tables
branch
from
efd1563 to
d563f05
Compare
Signed-off-by: Andreas Berger <andreas@berger-ecommerce.com>
github-actions
bot
added
area:techdocs
|
@vinzscam do you wanna have a peek through this? 🙏 |
| return ( | ||
| <Table<CatalogTableRow> | ||
| isLoading={loading} | ||
| <OffsetPaginatedCatalogTable |
There was a problem hiding this comment.
I'm not sure using OffsetPaginatedCatalogTable instead of Table is a good idea. Handling pagination properly will be on us and we don't have tests that cover that part. I would keep Table to stay safe.
There was a problem hiding this comment.
There are only cursor or offset based tables. Even if paging-mode is not explicitly set, the offset variant is used at the end. The separation, as it was until now, only makes the table behave differently in the case that paging is explicitly activated than if no explicit paging mode is specified.
There was a problem hiding this comment.
the Offset table contains some customization to make it work with server side paginated data. The normal <Table /> uses client side paginated data, which behaves differently.
There was a problem hiding this comment.
I've tested both with pagination mode offset and none, they behave as expected:
-
offset: server side paging
-
none: client side paging
plugins/catalog/src/components/CatalogTable/OffsetPaginatedCatalogTable.tsx
Show resolved
Hide resolved
Signed-off-by: Andreas Berger <andreas@berger-ecommerce.com>
Signed-off-by: Andreas Berger <andreas@berger-ecommerce.com>
Signed-off-by: Andreas Berger <andreas@berger-ecommerce.com>
Signed-off-by: Andreas Berger <andreas@berger-ecommerce.com>
There was a problem hiding this comment.
going through the proposed changes one by one:
- Show pagination text forOffsetPagination`
I think this is great
- Use sameOffsetPaginatedCatalogTablealso as fallback if no pagination is set
I feel like we should keep things as they are as this won't fix any bug or reduce any complexity. Defining an extra <Table /> in CatalogTable allows us to better split the logic for server side and client side paginated table.
- Do not show paging if there is only one page
if it's possible to change the number of items per page via the UI we should always show the paging information (right now if a user select to display a higher number of items per page than the items that are available, it won't be possible to change the limit anymore).
| }} | ||
| totalCount={totalItems} | ||
| localization={{ pagination: { labelDisplayedRows: '' } }} | ||
| {...(clientPagination |
There was a problem hiding this comment.
I would prefer to keep a duplicated <Table /> as it was before as it's make things easier if more things need to be adjusted.
There was a problem hiding this comment.
I disagree, the separation has already led to the CatalogTable behaving differently several times, e.g:
- currently: missing display of the pagination text
- fix: use default catalog toolbar for offset catalog table #27129 Here, for example, the toolbar was not built into ?
CatalogTable, which was also fixed in the next PR - Missing title Make title consistent across CatalogTable impls #27632
There was a problem hiding this comment.
I agree that this led us to miss some common props, but the advantage has been that we have introduced 3 different ways of managing and displaying data without affecting each other. Now we have a more complex OffsetPaginatedCatalogTable that handles server side and client side logic. How can we make sure that this change won't break the existing logic? Also in the future the goal is to sunset the client side paginated table, which is pretty straight forward at the moment as the logic isn't mixed with any of the two new tables.
There was a problem hiding this comment.
I agree that this led us to miss some common props, but the advantage has been that we have introduced 3 different ways of managing and displaying data without affecting each other.
I don't see it as an advantage that the server and client-side paging were handled in different places, as explained above, this has led to inconsistencies in the code on several occasions. With my adjustments, the paging behavior is now harmonized and always the same, whether server or client side.
Now we have a more complex OffsetPaginatedCatalogTable that handles server side and client side logic. How can we make sure that this change won't break the existing logic?
It's just an refactoring, that is even reducing the total lines of code. The added "complexity" is just an early return in the hook and a ternary for setting the server side config on the Table. You can simply compare the previous invocation of the client-side-paged table:
with the now called code of the OffsetPaginatedCatalogTable
the final call of Table is ultimately exactly the same as it was before, with the following adjustments:
- fix of paging text
- harmonized pages size selection (options for 5 and 10 are added)
- always visibility of the paging bar as requested by you in Harmonize
CatalogTable#28449 (review)
Also in the future the goal is to sunset the client side paginated table, which is pretty straight forward at the moment as the logic isn't mixed with any of the two new tables.
It is still easy to remove the client-side paging, because you can explicitly find the paging mode none here (which was not the case before). And in the end you only have to remove clientPagination, which is 2 places in this file, that's it.
…here is only one page available Signed-off-by: Andreas Berger <andreas@berger-ecommerce.com>
Hey, I just made a Pull Request!
Harmonize
CatalogTableby:OffsetPaginationOffsetPaginatedCatalogTablealso as fallback if no pagination is set✔️ Checklist
Signed-off-byline in the message. (more info)