Transaction Table Resize #3309
Transaction Table Resize #3309
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
|
Is this going to solve for #536? 🤞 |
it should since you can expand the column. |
lelemm
changed the title
…into table_resize_test
Fixed. This change may impact Visual Regression ( didn't test it locally), tomorrow I will check. I feel like this is near the finish. If more people can test this will be nice. |
|
Found 2 more inconsistencies. After these two probably this will be ready for review |
….tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
….tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (41)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-creates-a-new-account-and-views-the-initial-balance-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-creates-a-new-account-and-views-the-initial-balance-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-creates-a-new-account-and-views-the-initial-balance-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-9-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-9-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (2)
- packages/desktop-client/e2e/transactions.test.js (1 hunks)
- packages/desktop-client/src/components/ColumnWidthContext.jsx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/ColumnWidthContext.jsx (1)
Learnt from: lelemm PR: actualbudget/actual#3309 File: packages/desktop-client/src/components/ColumnWidthContext.jsx:17-17 Timestamp: 2024-10-25T18:30:24.287Z Learning: In `packages/desktop-client/src/components/ColumnWidthContext.jsx`, replacing `useState` with `useRef` for `positionAccumulator` causes the resize action to become unpredictable. Therefore, it's preferable to keep using `useState` for `positionAccumulator` in this context.
🔇 Additional comments (4)
packages/desktop-client/e2e/transactions.test.js (1)
53-55: Verify the intention behind changing the assertion method.The change from
toHaveTexttotoContainTextmakes the test less precise. While this might be necessary to accommodate the new resizable table implementation, consider:
- If possible, maintain the stricter
toHaveTextassertion to ensure exact matches- If
toContainTextis required, please document why the less strict assertion is necessary✅ Verification successful
Change from
toHaveTexttotoContainTextis appropriateThe change to
toContainTextis justified because:
- The "No transactions" text is rendered within nested components (as seen in Account.tsx and TransactionList.jsx)
- The text appears with additional markup and styling elements around it
- The assertion needs to be flexible enough to work with both desktop and mobile views, which render the message differently
The less strict assertion is necessary here and doesn't compromise test reliability since "No transactions" is a unique, specific message that wouldn't appear in other contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the "No transactions" text structure has changed # Look for the implementation of the empty state message in the new table component # Search for "No transactions" text rendering rg -l "No transactions" # Search for TableResizable component implementation ast-grep --pattern 'const TableResizable = $_'Length of output: 4255
Script:
#!/bin/bash # Let's check how "No transactions" is rendered in the Account component and transaction list rg -A 5 "No transactions" packages/desktop-client/src/components/accounts/Account.tsx packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx # Also check the implementation around the transaction table in the test file rg -B 5 -A 5 "transactionTable" packages/desktop-client/e2e/transactions.test.jsLength of output: 2508
packages/desktop-client/src/components/ColumnWidthContext.jsx (3)
190-196: LGTM! Well-implemented context hook.The hook follows React best practices with proper error handling for usage outside the provider context.
67-69: Ensure proper sanitization of columnName in querySelectorAll.The
columnNameparameter is used directly in a querySelectorAll selector. While this is within a controlled context, it's good practice to sanitize the input.
10-10: Verify the AUTO_SIZE constant value.The AUTO_SIZE constant is used to replace the magic number -1. Please ensure it's properly defined in the constants file.
✅ Verification successful
AUTO_SIZE constant is correctly defined and used
The AUTO_SIZE constant is properly defined in
loot-core/src/client/constants.tswith a value of -1 and is correctly imported and used inColumnWidthContext.jsxfor auto-sizing columns. The constant is also consistently used across other components liketable.tsxfor the same purpose.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AUTO_SIZE constant definition rg -A 1 "AUTO_SIZE.*=.*" "loot-core/client/constants"Length of output: 120
Script:
#!/bin/bash # Let's try to find the constants file with a broader search fd "constants.js|constants.ts" --type f # Also search for AUTO_SIZE usage across the codebase to understand its definition rg "AUTO_SIZE" --type js --type jsx --type ts --type tsxLength of output: 229
Script:
#!/bin/bash # Check the content of both constants files and search for AUTO_SIZE cat "packages/loot-core/src/client/constants.ts" cat "packages/desktop-client/src/components/reports/constants.ts" # Search for AUTO_SIZE with correct file extensions rg "AUTO_SIZE" -t js -t tsLength of output: 2433
|
Fixed all the problems that I found |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/ColumnWidthContext.jsx (1)
64-88: Consider documenting the setTimeout delay.The 100ms delay in
handleDoubleClickis a magic number. Consider adding a comment explaining why this specific delay is necessary for DOM measurements.- }, 100); + // Wait for the DOM to update after setting AUTO_SIZE + }, 100);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/ColumnWidthContext.jsx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/ColumnWidthContext.jsx (1)
Learnt from: lelemm PR: actualbudget/actual#3309 File: packages/desktop-client/src/components/ColumnWidthContext.jsx:17-17 Timestamp: 2024-10-25T18:30:24.287Z Learning: In `packages/desktop-client/src/components/ColumnWidthContext.jsx`, replacing `useState` with `useRef` for `positionAccumulator` causes the resize action to become unpredictable. Therefore, it's preferable to keep using `useState` for `positionAccumulator` in this context.
🔇 Additional comments (4)
packages/desktop-client/src/components/ColumnWidthContext.jsx (4)
1-15: LGTM! Well-organized imports and context creation.The imports are comprehensive and properly structured, with a clear separation between React core imports, constants, and custom hooks.
192-198: LGTM! Well-implemented context hook.The
useColumnWidthhook follows React best practices by:
- Properly consuming the context
- Providing a descriptive error message when used outside the provider
92-134: 🛠️ Refactor suggestionReuse memoized width calculations.
The
updateColumnWidthfunction recalculatescurrentTotalWidthandotherColumnsWidthwhich are already memoized at the component level.const updateColumnWidth = useCallback( (columnName, accumulatedDelta) => { const existingColumnWidth = columnWidths[columnName] || 0; const newWidth = accumulatedDelta; if (newWidth === AUTO_SIZE) { const newObj = { ...columnWidths, [columnName]: newWidth, }; setColumnWidths(newObj); return newObj; } - const currentTotalWidth = Object.values(columnWidths).reduce( - (acc, width) => acc + width, - 0, - ); - const otherColumnsWidth = Object.values(fixedSizedColumns).reduce( - (acc, width) => acc + width + 10, - 0, - ); const proposedTotalWidth = currentTotalWidth - existingColumnWidth + newWidth + otherColumnsWidth;Likely invalid or redundant comment.
69-71: Ensure columnName is properly sanitized.Using template literals in
querySelectorAllcould be vulnerable to XSS ifcolumnNamecontains malicious content. Consider implementing input validation or using a safer DOM querying approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/desktop-client/src/style/styles.ts (2)
164-171: LGTM: Improved selector scoping with documentation suggestion.The updated scrollbar styles use proper nesting with the
&selector and maintain a clean visual appearance with transparent backgrounds.Consider adding a comment explaining the purpose of these specific styles, especially how they relate to the table resizing feature.
+ // These styles provide a minimal scrollbar appearance for better visual integration + // with the resizable table feature styles.horizontalScrollbar = !hiddenScrollbars && {
173-180: Consider dark mode compatibility for table scrollbars.The table-specific horizontal scrollbar styles look good, but consider adding dark mode variants to ensure visibility across themes. The current background color (
rgba(200, 200, 200, .5)) might not provide sufficient contrast in dark mode.styles.horizontalScrollbarTable = !hiddenScrollbars && { '& ::-webkit-scrollbar:horizontal': { height: 11, backgroundColor: 'rgba(0, 0, 0, 0)', }, '& ::-webkit-scrollbar-thumb:horizontal': { - backgroundColor: 'rgba(200, 200, 200, .5)', + backgroundColor: theme.mode === 'dark' + ? 'rgba(200, 200, 200, .7)' + : 'rgba(200, 200, 200, .5)', }, };packages/desktop-client/src/components/table.tsx (3)
Line range hint
164-295: Fix duplicate dependency in Cell component's useMemo hookThe
useMemohook's dependency array includesexposedtwice, which is unnecessary and could potentially cause unnecessary re-renders.Apply this diff to fix the dependency array:
[ privacyFilter, focused, exposed, children, plain, - exposed, valueStyle, onExpose, name, unexposedContent, value, formatter, ]
1307-1314: Consider optimizing scroll synchronizationThe current implementation makes multiple scroll calls in the onScroll handler. Consider combining the scroll updates for better performance.
Consider refactoring to:
- subHeaderRef?.current.scrollTo({ - left: scrollContainer.current.scrollLeft, - behavior: 'instant', - }); - scrollContainerHeader?.current.scrollTo({ - left: scrollContainer.current.scrollLeft, - behavior: 'instant', - }); + const scrollLeft = scrollContainer.current.scrollLeft; + [subHeaderRef, scrollContainerHeader].forEach(ref => { + ref?.current?.scrollTo({ + left: scrollLeft, + behavior: 'instant' + }); + });
1020-1024: Add documentation for subHeader offset calculationThe subHeader offset calculation logic would benefit from documentation explaining its purpose and when it's recalculated.
Consider adding a comment like:
+ // Calculate and update the subHeader's offset height whenever it changes + // This offset is used to adjust the main content area's height useEffect(() => { if (subHeaderRef.current) { setSubHeaderOffset(subHeaderRef.current.offsetHeight); } }, [subHeaders, subHeaderRef]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/desktop-client/src/components/FixedSizeList.tsx(8 hunks)packages/desktop-client/src/components/table.tsx(19 hunks)packages/desktop-client/src/style/styles.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/FixedSizeList.tsx
🔇 Additional comments (6)
packages/desktop-client/src/style/styles.ts (2)
150-150: LGTM: New style variable follows established patterns.
The new horizontalScrollbarTable variable is properly typed and follows the existing naming conventions.
Line range hint 208-219: Verify horizontal scrollbar detection.
The testScrollbars function only checks vertical scrollbar visibility. Consider extending it to verify horizontal scrollbar visibility as well, since the new table resizing feature relies heavily on horizontal scrolling.
packages/desktop-client/src/components/table.tsx (4)
23-24: LGTM: New imports support resizable columns feature
The added imports for AUTO_SIZE, useMergedRefs, and ColumnWidthProvider are appropriate for implementing the resizable columns functionality.
Also applies to: 25-25, 36-36
415-434: LGTM: InputCell component refactoring
The InputCell component has been properly refactored to use forwardRef and maintains clean prop handling.
459-513: LGTM: CustomCell component refactoring
The CustomCell component has been properly refactored to use forwardRef with good state management and event handling.
1333-1339: LGTM: TableResizable component implementation
The TableResizable component correctly wraps the Table component with ColumnWidthProvider for column width management.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/table.tsx (2)
1207-1207: Document the magic number for scrollbar width.The hardcoded value
11for scrollbar width should be documented or extracted into a named constant for better maintainability.+// Default width of the vertical scrollbar in pixels +const SCROLLBAR_WIDTH = 11; + <View style={{ - width: `${totalWidth() + 11 /* 11 = vertical scrollbar width */}px`, + width: `${totalWidth() + SCROLLBAR_WIDTH}px`, }} >
1304-1314: Consider optimizing scroll synchronization.The scroll synchronization between header, subheader, and content could benefit from debouncing to improve performance, especially during rapid scrolling.
Consider using a debounced version of the scroll handler:
import { debounce } from 'lodash'; // Create a debounced scroll handler const debouncedScrollSync = debounce((scrollLeft) => { subHeaderRef?.current.scrollTo({ left: scrollLeft, behavior: 'instant', }); scrollContainerHeader?.current.scrollTo({ left: scrollLeft, behavior: 'instant', }); }, 16); // 16ms = 1 frame at 60fps // Use in onScroll onScroll={config => { onScroll?.(); debouncedScrollSync(scrollContainer.current.scrollLeft); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/table.tsx(19 hunks)
🔇 Additional comments (3)
packages/desktop-client/src/components/table.tsx (3)
23-24: LGTM: New imports support resizable columns feature.
The added imports for AUTO_SIZE, useMergedRefs, and column width management utilities are appropriate for implementing the resizable columns feature.
Also applies to: 36-36
164-184: LGTM: Components properly refactored to use forwardRef.
The refactoring of Cell, InputCell, and CustomCell components to use forwardRef improves ref handling and TypeScript type safety. The implementation follows React best practices.
Also applies to: 415-434, 459-513
1189-1216: LGTM: Robust AutoSizer integration with proper edge case handling.
The AutoSizer integration is well implemented with:
- Proper handling of zero width/height cases
- Correct width calculations for headers and content
- Appropriate resize event handling
Also applies to: 1249-1323
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
packages/desktop-client/src/components/transactions/SimpleTransactionsTable.jsx (2)
Line range hint
179-248: Implement column flexibility requirementsAccording to the PR discussion:
- The NOTES column should be flexible while others have fixed sizes
- Resizing non-NOTES columns should affect the NOTES column's remaining space
Consider adding these configurations:
<TableResizable prefName="simple-transactions-column-sizes" style={style} items={serializedTransactions} renderEmpty={renderEmpty} autoSizer={false} + flexibleColumns={['notes']} headers={
Line range hint
179-248: Consider implementing a column management systemTo address the various UX concerns raised in the PR discussion, consider implementing a comprehensive column management system that includes:
- A column configuration context to manage column states
- A user preferences service to persist column settings
- A UI component for column management (reset, visibility, order)
- Documentation for column resize features
This would provide a more maintainable and user-friendly solution for handling column-related functionality.
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx (1)
150-151: Add column reset functionalityBased on the PR objectives, users have requested the ability to reset column sizes. Consider:
- Adding a reset button in the modal header
- Including tooltip or documentation about the right-click reset feature mentioned in the PR comments
Example implementation:
<ModalHeader title={t('Link Accounts')} rightContent={ <View style={{ display: 'flex', gap: 8 }}> <Button variant="secondary" title={t('Reset column sizes')} onPress={() => { // Add reset logic here }} /> <ModalCloseButton onPress={close} /> </View> } />packages/desktop-client/src/components/table.tsx (2)
1304-1314: Consider optimizing scroll synchronization performance.The current scroll synchronization implementation might cause performance issues:
- Multiple scroll event handlers firing on each scroll
- No debouncing of scroll events
- Instant scroll behavior might cause visual jank
Consider these improvements:
+ const handleScroll = useCallback( + debounce(() => { + const { scrollLeft } = scrollContainer.current; + requestAnimationFrame(() => { + if (subHeaderRef?.current) { + subHeaderRef.current.scrollTo({ left: scrollLeft }); + } + if (scrollContainerHeader?.current) { + scrollContainerHeader.current.scrollTo({ left: scrollLeft }); + } + }); + onScroll?.(); + }, 16), + [onScroll] + ); <FixedSizeList // ...other props - onScroll={() => { - onScroll?.(); - subHeaderRef?.current.scrollTo({ - left: scrollContainer.current.scrollLeft, - behavior: 'instant', - }); - scrollContainerHeader?.current.scrollTo({ - left: scrollContainer.current.scrollLeft, - behavior: 'instant', - }); - }} + onScroll={handleScroll} />
1189-1220: Simplify AutoSizer header implementation.The current implementation has unnecessary conditional logic and nested views. Consider simplifying it for better maintainability.
- let encapsulatedHeaders: ReactNode | TableHeaderProps['headers'] = - autoSizer && ( - <AutoSizer disableHeight={true}> - {({ width }) => { - if (width === 0) { - return null; - } - return ( - <View - ref={scrollContainerHeader} - style={{ - width: `${width}px`, - overflow: 'hidden', - }} - > - <View - style={{ - width: `${totalWidth() + 11}px`, - }} - > - {headers} - </View> - </View> - ); - }} - </AutoSizer> - ); - - if (!autoSizer) { - encapsulatedHeaders = headers; - } + const HeaderContent = ({ width }) => ( + width === 0 ? null : ( + <View + ref={scrollContainerHeader} + style={{ + width: `${width}px`, + overflow: 'hidden', + }} + > + <View style={{ width: `${totalWidth() + 11}px` }}> + {headers} + </View> + </View> + ) + ); + + const encapsulatedHeaders = autoSizer ? ( + <AutoSizer disableHeight>{HeaderContent}</AutoSizer> + ) : headers;packages/desktop-client/src/components/accounts/Account.tsx (1)
730-732: Fix typo in method nameThe method name contains a typo: "Toogle" should be "Toggle".
Apply this diff to fix the typo:
- onToogleColumnEdit = () => { + onToggleColumnEdit = () => { this.props.toggleColumnEditMode(); }; onMenuSelect = async ( item: | 'link' | 'unlink' | 'close' | 'reopen' | 'export' | 'toggle-balance' | 'remove-sorting' | 'toggle-cleared' | 'toggle-reconciled' - | 'toogle-column-edit', + | 'toggle-column-edit', ) => { // ... switch (item) { // ... - case 'toogle-column-edit': + case 'toggle-column-edit': - this.onToogleColumnEdit(); + this.onToggleColumnEdit(); break;Also applies to: 833-835
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (4)
2050-2070: Consider memoizing the column width configurationThe column width configuration is recreated on every render due to the useEffect dependency on
columnWidthsandcolumnWidths.current. This could lead to unnecessary re-renders.Consider memoizing the fixed column widths configuration:
- useEffect(() => { + const fixedColumnConfig = useMemo(() => ({ + notes: 'flex', + selected: 20, + date: 110, + debit: 100, + credit: 100, + payee: 200, + account: 200, + category: 110, + balance: props.showBalance ? 103 : 0, + cleared: props.showCleared ? 38 : 0, + }), [props.showBalance, props.showCleared]); + + useEffect(() => { - setFixedSizedColumns({ - notes: 'flex', - selected: 20, - date: 110, - debit: 100, - credit: 100, - payee: 200, - account: 200, - category: 110, - balance: props.showBalance ? 103 : 0, - cleared: props.showCleared ? 38 : 0, - }); + setFixedSizedColumns(fixedColumnConfig); }, [columnWidths, columnWidths.current]);
Line range hint
234-343: Refactor ref assignments to improve code clarityMultiple instances of ref assignments in expressions were flagged by static analysis. While functional, this pattern can make the code harder to maintain and understand.
Consider refactoring the ref assignments to be more explicit:
- ref={el => (refs.current['date'] = el)} + ref={el => { + refs.current['date'] = el; + }}This pattern should be applied to all similar ref assignments in the code.
Also applies to: 1286-1737
🧰 Tools
🪛 Biome
[error] 295-295: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 315-315: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
1387-1421: Consider extracting PayeeCell rendering logicThe PayeeCell rendering logic is complex and could impact performance. Consider extracting it into a separate component to improve maintainability and enable better optimization.
Extract the PayeeCell rendering logic into a separate component:
const PayeeCellRenderer = memo(({ transaction, columnWidths, ...props }) => { return ( <> <PayeeCell id={transaction.id} width={columnWidths.payee} {...props} /> <Field width={10} style={{ width: 10, }} /> </> ); });This would allow for better memoization and reduce unnecessary re-renders.
🧰 Tools
🪛 Biome
[error] 1392-1392: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Line range hint
2188-2268: Add error boundaries for transaction table renderingThe transaction table handles complex state and rendering logic but lacks error boundaries to gracefully handle rendering failures.
Consider adding an error boundary component:
class TransactionTableErrorBoundary extends React.Component { state = { hasError: false }; static getDerivedStateFromError(error) { return { hasError: true }; } render() { if (this.state.hasError) { return <div>Error loading transaction table. Please refresh the page.</div>; } return this.props.children; } } // Wrap the table content <TransactionTableErrorBoundary> <View style={{ flex: 1, overflowX: 'hidden', position: 'relative', }} data-testid="transaction-table" > {/* ... existing content ... */} </View> </TransactionTableErrorBoundary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
packages/desktop-client/src/components/FixedSizeList.tsx(8 hunks)packages/desktop-client/src/components/accounts/Account.tsx(10 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx(2 hunks)packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx(2 hunks)packages/desktop-client/src/components/payees/PayeeTable.tsx(2 hunks)packages/desktop-client/src/components/schedules/SchedulesTable.tsx(2 hunks)packages/desktop-client/src/components/table.tsx(18 hunks)packages/desktop-client/src/components/transactions/SimpleTransactionsTable.jsx(2 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(36 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx(4 hunks)packages/desktop-client/src/style/styles.ts(3 hunks)packages/loot-core/src/types/prefs.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
- packages/desktop-client/src/components/payees/PayeeTable.tsx
- packages/desktop-client/src/components/schedules/SchedulesTable.tsx
- packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx
- packages/desktop-client/src/style/styles.ts
- packages/loot-core/src/types/prefs.d.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/accounts/Account.tsx (3)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-11-10T16:45:31.225Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🪛 Biome
packages/desktop-client/src/components/transactions/TransactionsTable.jsx
[error] 234-234: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 249-249: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 267-267: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 280-280: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 295-295: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 315-315: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 328-328: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 343-343: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1286-1286: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1334-1334: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1392-1392: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1425-1425: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1452-1452: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1546-1546: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1576-1576: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1657-1657: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1692-1692: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1737-1737: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (11)
packages/desktop-client/src/components/transactions/SimpleTransactionsTable.jsx (2)
23-23: LGTM: Import change aligns with resize functionality
The change from Table to TableResizable is consistent with the PR's objective to add column resizing capabilities.
179-184: Consider adding column reset functionality
Based on the PR discussion, users need a way to reset column sizes. While right-click reset is mentioned, consider:
- Adding a visible reset button or menu item
- Documenting the reset functionality in tooltips or help text
Verify horizontal scrolling behavior
Setting autoSizer={false} might contribute to the horizontal scrolling issues mentioned in the PR comments.
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx (3)
21-21: LGTM: Clean import change
The import statement correctly includes the new TableResizable component while maintaining the existing imports.
Line range hint 152-174: LGTM: Clean integration with existing functionality
The TableResizable component maintains all the necessary props and functionality from the original implementation:
- Proper key handling for list items
- Consistent rendering of TableRow components
- Preserved account linking/unlinking logic
150-151: Consider potential layout issues with fixed widths
The table is contained within a modal of fixed width (800px) and includes columns with fixed widths (200px + 80px) plus two flex columns. This combination might:
- Cause horizontal scrolling if columns are resized beyond the modal width
- Create layout issues when the two flex columns compete for remaining space
Let's verify the column width calculations:
packages/desktop-client/src/components/table.tsx (2)
23-24: LGTM: New imports are well-organized.
The new imports are properly structured and necessary for implementing the resizable columns feature.
Also applies to: 25-25, 36-36
164-184: LGTM: Components properly refactored with forwardRef.
The Cell, InputCell, and CustomCell components have been correctly refactored to use forwardRef, improving their reusability and following React best practices. The type definitions are properly maintained.
Also applies to: 415-434, 459-513
packages/desktop-client/src/components/accounts/Account.tsx (3)
72-72: LGTM: Column width management integration
The addition of column width management through context and hooks is well-structured and follows React best practices.
Also applies to: 288-288
1851-1851: LGTM: Column width hook integration
The column width management hook is properly integrated into the component hierarchy.
Also applies to: 1861-1861
1868-1873: LGTM with a note about potential conflicts
The column width context integration is well-structured and follows React best practices. However, be aware that this component is being rewritten in a separate PR, which might lead to merge conflicts.
Let's verify the status of the rewrite PR:
Also applies to: 1875-1916, 1953-1953
packages/desktop-client/src/components/FixedSizeList.tsx (1)
273-273: Consider consistent unit handling and clarify subtraction of 16 pixels
The width calculation subtracts 16 pixels: ${Math.max(totalWidth || 0, width || 0) - 16}px. This approach might lead to negative widths if both totalWidth and width are less than 16. Additionally, the reason for subtracting 16 pixels is unclear.
Previous comment still applies: ensure consistent unit handling for width and totalWidth, and consider explaining or adjusting the subtraction of 16 pixels to prevent potential issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/table.tsx (1)
1020-1024: Consider using ResizeObserver instead of useEffect for header height.The current implementation uses useEffect to measure subHeader height, which could cause layout shifts. Consider using ResizeObserver for more reliable measurements.
-useEffect(() => { - if (subHeaderRef.current) { - setSubHeaderOffset(subHeaderRef.current.offsetHeight); - } -}, [subHeaders, subHeaderRef]); +useEffect(() => { + if (subHeaderRef.current) { + const resizeObserver = new ResizeObserver(entries => { + for (const entry of entries) { + setSubHeaderOffset(entry.contentRect.height); + } + }); + resizeObserver.observe(subHeaderRef.current); + return () => resizeObserver.disconnect(); + } +}, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (51)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-10-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-11-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-12-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-9-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-9-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
packages/desktop-client/src/components/table.tsx(18 hunks)
🔇 Additional comments (5)
packages/desktop-client/src/components/table.tsx (5)
23-24: LGTM: New imports support column resizing functionality.
The added imports are appropriate for implementing the resizable columns feature:
AUTO_SIZEconstant for dynamic column sizinguseMergedRefshook for ref managementColumnWidthProvideranduseColumnWidthfor column width management
Also applies to: 25-25, 36-36
Line range hint 164-295: LGTM: Cell component refactored with proper ref forwarding.
The Cell component has been properly refactored to:
- Use forwardRef for better ref handling
- Include comprehensive TypeScript types
- Implement proper ref merging with useMergedRefs
415-434: LGTM: InputCell component refactored with proper ref forwarding.
The InputCell component refactoring looks good with:
- Proper ref forwarding implementation
- Clean TypeScript types
- Consistent props handling
459-513: LGTM: CustomCell component refactored with proper ref forwarding.
The CustomCell component refactoring is well done with:
- Proper ref forwarding
- Clear state management
- Consistent event handling
1331-1341: LGTM: TableResizable component implementation.
The TableResizable component is well implemented:
- Properly wraps Table with ColumnWidthProvider
- Correctly forwards refs
- Has appropriate TypeScript generics
|
/update-vrt |
Created a transaction table resize on this PR.
check this comment #3309 (comment) for a video of it working