OBPIH-6845 Create table component all products tab#4982
Conversation
alannadolny
requested review from
awalkowiak,
drodzewicz,
ewaterman and
kchelstowski
github-actions
bot
added
domain: frontend
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4982 +/- ##
============================================
+ Coverage 7.60% 7.68% +0.07%
- Complexity 817 834 +17
============================================
Files 601 601
Lines 42342 42355 +13
Branches 10284 10289 +5
============================================
+ Hits 3222 3254 +32
+ Misses 38652 38621 -31
- Partials 468 480 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| className={`rt-th ${className}`} | ||
| > | ||
| {children} | ||
| {sortable && ( |
There was a problem hiding this comment.
just out of curiosity - how was it hidden in the old version?
There was a problem hiding this comment.
there was additional styling applied, but I think the current version is much more straightforward
| defaultMessage={ | ||
| emptyTableMessage?.defaultMessage || defaultEmptyTableMessage.defaultMessage | ||
| } | ||
| shouldDisplay={!dataLength && !loading} |
There was a problem hiding this comment.
I don't mind doing it this way, but the way I would prefer is to conditionally render those, so:
{(!dataLength && !loading) && <DataTableStatus .... />}
{loading && <DataTableStatus />}but it's not a change request
| const data = [ | ||
| { | ||
| // other properties | ||
| lastCountDate: '12/11/2024', |
There was a problem hiding this comment.
I don't expect you to waste time to change that now, especially that the backend is almost ready, but this property will be named dateLastCounted
There was a problem hiding this comment.
yeah, it will be changed when integrating with the backend
| </TableCell> | ||
| ), | ||
| }), | ||
| ], []); |
There was a problem hiding this comment.
shouldn't translate be the dependency here?
There was a problem hiding this comment.
I can add it, but I am not sure how it will work, because translate is a function, so I don't think it will be working properly, it should rather be a selected language
There was a problem hiding this comment.
good point, I forgot that translate is a function. I think it's good to investigate places where we put it in the dependency, as we might have some unexpected rerenders there (still it's better to have it, than not to have it, as we would have a bug). And yeah, selected language is the option here.
|
|
||
| import { getCoreRowModel, getPaginationRowModel, useReactTable } from '@tanstack/react-table'; | ||
|
|
||
| const useDataTable = ({ pageSize, columns, data }) => { |
There was a problem hiding this comment.
we have two similar hooks now - useTableData and useDataTable - can we easily say what the differences are and maybe come up with a better name for this one?
P.S. I don't expect you to explain it to me, because I see the difference, but I rather meant when someone tackled this for the first time and started wondering what the differences are.
There was a problem hiding this comment.
I don't have any better idea for this hook
alannadolny
force-pushed
the
OBPIH-6845
branch
from
3d0e835 to
78dad16
Compare
3 participants
No description provided.