OBPIH-6846 Build checkbox logic#4999
Conversation
github-actions
bot
added
domain: frontend
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4999 +/- ##
=========================================
Coverage 7.69% 7.69%
Complexity 846 846
=========================================
Files 610 610
Lines 42586 42586
Branches 10344 10344
=========================================
Hits 3275 3275
Misses 38826 38826
Partials 485 485 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| .header-cell { | ||
|
|
There was a problem hiding this comment.
nitpicky: unnecessary indentation
|
|
||
| // Separated from columns to reduce the amount of rerenders of | ||
| // the rest columns (on checked checkboxes change) | ||
| const checkboxesColumn = columnHelper.accessor('selected', { |
There was a problem hiding this comment.
do you think there would be any benefit from including the useMemo here not to re-render it every single time, since a lot of compoments are built here?
There was a problem hiding this comment.
There are benefits, because I can't use useMemo on all columns, because there are problems with refreshing the checkboxes (even with correct dependencies)
| headerCheckboxProps, | ||
| } = useTableCheckboxes(); | ||
|
|
||
| const getProductIds = () => tableData.data.map((row) => row.product.id); |
There was a problem hiding this comment.
I guess this doesn't even have to be a method, but could be a const, like:
const productIds = useMemo(() => tableData.data.map(...), [<any dependency indicating the tableData changed>])I don't know though how the re-rendering of methods is executed for such cases, whether the mapping would be proceeded in the background while re-rendering or not.
src/js/hooks/useTableCheckboxes.js
Outdated
| }, [checkedCheckboxes]); | ||
|
|
||
| useEffect(() => { | ||
| const selectedRows = checkedCheckboxes.length; |
There was a problem hiding this comment.
I think you mix two namings here - selectedRows vs checkedCheckboxes. Maybe we could align on one of them?
Moreover - for me it isn't a problem, but selectedRows sound like we store the whole rows there, not only the length, so how about adding any amountOf prefix?
alannadolny
force-pushed
the
OBPIH-6846
branch
from
4d466cb to
eb5ab59
Compare
2 participants
✨ Description of Change
Link to GitHub issue or Jira ticket:
Description: I modified the checkbox component to be able to use an indeterminate state. I also added a hook for managing checkboxes in the table.
📷 Screenshots & Recordings (optional)