(typescript) Refactoring the mobile TransactionListWithBalance component into typescript. #4061
Conversation
actual-github-bot
bot
changed the title
✅ 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
|
| >, | ||
| ) => { | ||
| return <CellValue {...props} />; | ||
| }; |
There was a problem hiding this comment.
Following the pattern found in budget/envelope/EnvelopeBudgetCompontns.tsx and budget/tracking/TrackingBudgetCompontns.tsx
| </View> | ||
| <PullToRefresh | ||
| isPullable={!!onRefresh} | ||
| onRefresh={async () => onRefresh?.()} |
There was a problem hiding this comment.
this props accept an async function
leoltl
changed the title
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request involves the removal of the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx (5)
25-69: Accessibility nitpick forTransactionSearchInput
Consider adding anaria-labeloraria-describedbyattribute to improve screen reader accessibility, particularly if the placeholder is omitted or not descriptive.<InputWithContent + aria-label={placeholder} ... />
71-93: Complex union types
TheTransactionListWithBalancesPropstype comprehensively covers multiple union types for different balance queries. Consider extracting separate smaller types or using discriminated unions to simplify future maintenance.
95-107: HandlingisLoading
It might be beneficial to render a loading indicator (e.g., a spinner) whenisLoadingis true, improving user experience during data fetches.
157-168: Optional naming improvement
TransactionListBalanceCellValueis descriptive but somewhat lengthy. If commonly used, consider a shorter alias or internal variable to boost readability in repeated usage.
180-249: Duplicate patterns inBalanceWithCleared
Cleared and uncleared sections have similar layout code. A shared internal helper could reduce duplication and centralize style transformations.function BalanceWithCleared({...}) { - // Repeats code for cleared & uncleared + // Possible approach: factor out repeated code for each balance section into a sub-component or method }packages/desktop-client/src/components/spreadsheet/index.ts (1)
18-26: Consistent naming incategory
All fields are named consistently with the existing structure. A minor suggestion is to confirm that'uncategorized-amount'and'uncategorized-balance'align with other naming conventions (e.g.,'offbudget-accounts-balance'). If everything is uniform across the codebase, it’s good to keep it as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4061.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx(0 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx(1 hunks)packages/desktop-client/src/components/spreadsheet/index.ts(1 hunks)packages/loot-core/src/client/queries.ts(5 hunks)
💤 Files with no reviewable changes (1)
- packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx (2)
Learnt from: matt-fidd
PR: actualbudget/actual#3472
File: packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx:375-383
Timestamp: 2024-11-10T16:45:25.627Z
Learning: Styles in the codebase are generally defined inline within components to match existing conventions and maintain readability. Extracting styles into constants may diverge from the codebase conventions.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx:274-274
Timestamp: 2024-11-10T16:45:25.627Z
Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
🔇 Additional comments (11)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx (6)
1-3: Imports look good
No immediate issues are found with the import statements or the usage of typed imports from React.
4-6: Imports from queries
This import style is consistent with the rest of the codebase. Nothing to change.
7-16: Imports for hooks, icons, and styling
Everything appears organized; the codebase’s inline styling convention is respected.
20-23: Type referencing with TransactionListWithBalancesProps['onSearch']
Using a prop reference to ensure consistent function signatures is a solid approach.
110-153: Inline styles and layout
Inline styles are consistent with the established convention in this project (per retrieved learnings). However, consider factoring out repeated style objects (e.g., flex containers) into local constants if usage grows across the file.
252-278: Overall design of Balance
The code correctly handles negative amounts by coloring them as error text and otherwise highlighting them. This approach is clear and user-friendly.
packages/loot-core/src/client/queries.ts (5)
28-28: Introduction of categoryParametrizedField
This is a clean solution to generate fields for category-related bindings.
183-191: categoryBalance usage
The approach for filtering transactions by category and month is straightforward and type-safe.
Line range hint 199-208: categoryBalanceCleared
Identical structure to categoryBalance but for cleared transactions. Nothing problematic here.
Line range hint 216-225: categoryBalanceUncleared
Same reliable pattern, focusing on uncleared transactions.
239-243: Generic uncategorizedBalance
Making this function generic with <SheetName extends SheetNames> is a neat enhancement, ensuring it integrates consistently with the rest of the code.
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx (4)
54-57: Consider implementing debounce for search functionalityThe search callback is triggered on every keystroke, which might cause performance issues with frequent API calls. Consider implementing debouncing to limit the rate of search requests.
+import { debounce } from 'lodash'; + function TransactionSearchInput({ placeholder, onSearch, }: TransactionSearchInputProps) { const [text, setText] = useState(''); + const debouncedSearch = React.useMemo( + () => debounce((searchText: string) => onSearch(searchText), 300), + [onSearch] + ); + + React.useEffect(() => { + return () => { + debouncedSearch.cancel(); + }; + }, [debouncedSearch]); return ( // ... rest of the component <InputWithContent value={text} onChangeValue={text => { setText(text); - onSearch(text); + debouncedSearch(text); }}
141-141: Simplify async callback and improve type safetyThe async arrow function with optional chaining can be simplified while maintaining type safety.
- onRefresh={async () => onRefresh?.()} + onRefresh={onRefresh}
156-167: Simplify generic type constraintsThe generic type constraint and component props type could be simplified while maintaining type safety.
-const TransactionListBalanceCellValue = < - FieldName extends SheetFields<'account'> | SheetFields<'category'>, ->( - props: ComponentProps< - typeof CellValue< - FieldName extends SheetFields<'account'> ? 'account' : 'category', - FieldName - > - >, -) => { +type ValidSheetName = 'account' | 'category'; +type ValidFieldName = SheetFields<ValidSheetName>; + +const TransactionListBalanceCellValue = <T extends ValidFieldName>( + props: ComponentProps<typeof CellValue<ValidSheetName, T>>, +) => { return <CellValue {...props} />; };
124-133: Consider extracting balance display logicThe conditional rendering of balance components could be extracted into a separate component to improve maintainability and reusability.
Consider creating a
BalanceDisplaycomponent that encapsulates the logic for choosing betweenBalanceWithClearedandBalancecomponents. This would make the main component cleaner and the balance display logic more maintainable.type BalanceDisplayProps = { balance: TransactionListWithBalancesProps['balance']; balanceCleared?: TransactionListWithBalancesProps['balanceCleared']; balanceUncleared?: TransactionListWithBalancesProps['balanceUncleared']; }; function BalanceDisplay({ balance, balanceCleared, balanceUncleared }: BalanceDisplayProps) { return balanceCleared && balanceUncleared ? ( <BalanceWithCleared balance={balance} balanceCleared={balanceCleared} balanceUncleared={balanceUncleared} /> ) : ( <Balance balance={balance} /> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx (2)
Learnt from: matt-fidd
PR: actualbudget/actual#3472
File: packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx:375-383
Timestamp: 2024-11-10T16:45:25.627Z
Learning: Styles in the codebase are generally defined inline within components to match existing conventions and maintain readability. Extracting styles into constants may diverge from the codebase conventions.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx:274-274
Timestamp: 2024-11-10T16:45:25.627Z
Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
1st OSS PR, so apologies if there are stuff that I am missing. 😄
Refactoring the mobile TransactionListWithBalance component into typescript.
re #1483.