-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
(typescript) Refactoring the mobile TransactionList component to typescript #4063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
ce3b81d
a2efa90
dbc86ed
56eba2e
5623c21
81295e6
c06df65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import React, { | |
useMemo, | ||
useRef, | ||
useState, | ||
type CSSProperties, | ||
} from 'react'; | ||
import { ListBox, Section, Header, Collection } from 'react-aria-components'; | ||
import { useTranslation } from 'react-i18next'; | ||
|
@@ -12,6 +13,7 @@ import { setNotificationInset } from 'loot-core/client/actions'; | |
import { groupById, integerToCurrency } from 'loot-core/shared/util'; | ||
import * as monthUtils from 'loot-core/src/shared/months'; | ||
import { isPreviewId } from 'loot-core/src/shared/transactions'; | ||
import { type TransactionEntity } from 'loot-core/types/models/transaction'; | ||
|
||
import { useAccounts } from '../../../hooks/useAccounts'; | ||
import { useCategories } from '../../../hooks/useCategories'; | ||
|
@@ -29,7 +31,7 @@ import { SvgDotsHorizontalTriple } from '../../../icons/v1'; | |
import { useDispatch } from '../../../redux'; | ||
import { styles, theme } from '../../../style'; | ||
import { Button } from '../../common/Button2'; | ||
import { Menu } from '../../common/Menu'; | ||
import { Menu, type MenuItemObject } from '../../common/Menu'; | ||
import { Popover } from '../../common/Popover'; | ||
import { Text } from '../../common/Text'; | ||
import { View } from '../../common/View'; | ||
|
@@ -40,7 +42,12 @@ import { TransactionListItem } from './TransactionListItem'; | |
|
||
const NOTIFICATION_BOTTOM_INSET = 75; | ||
|
||
function Loading({ style, 'aria-label': ariaLabel }) { | ||
type LoadingProps = { | ||
style?: CSSProperties; | ||
'aria-label': string; | ||
}; | ||
|
||
function Loading({ style, 'aria-label': ariaLabel }: LoadingProps) { | ||
return ( | ||
<View | ||
aria-label={ariaLabel || 'Loading...'} | ||
|
@@ -57,17 +64,29 @@ function Loading({ style, 'aria-label': ariaLabel }) { | |
); | ||
} | ||
|
||
type TransactionListProps = { | ||
isLoading: boolean; | ||
transactions: readonly TransactionEntity[]; | ||
onOpenTransaction?: (transaction: TransactionEntity) => void; | ||
isLoadingMore: boolean; | ||
onLoadMore: () => void; | ||
}; | ||
|
||
export function TransactionList({ | ||
isLoading, | ||
transactions, | ||
onOpenTransaction, | ||
isLoadingMore, | ||
onLoadMore, | ||
}) { | ||
}: TransactionListProps) { | ||
const { t } = useTranslation(); | ||
const sections = useMemo(() => { | ||
// Group by date. We can assume transactions is ordered | ||
const sections = []; | ||
const sections: { | ||
id: string; | ||
date: TransactionEntity['date']; | ||
transactions: TransactionEntity[]; | ||
}[] = []; | ||
transactions.forEach(transaction => { | ||
if ( | ||
sections.length === 0 || | ||
|
@@ -88,13 +107,16 @@ export function TransactionList({ | |
const dispatchSelected = useSelectedDispatch(); | ||
const selectedTransactions = useSelectedItems(); | ||
|
||
const onTransactionPress = useCallback( | ||
const onTransactionPress: ( | ||
transaction: TransactionEntity, | ||
isLongPress?: boolean, | ||
) => void = useCallback( | ||
(transaction, isLongPress = false) => { | ||
const isPreview = isPreviewId(transaction.id); | ||
if (!isPreview && (isLongPress || selectedTransactions.size > 0)) { | ||
dispatchSelected({ type: 'select', id: transaction.id }); | ||
} else { | ||
onOpenTransaction(transaction); | ||
onOpenTransaction?.(transaction); | ||
} | ||
}, | ||
[dispatchSelected, onOpenTransaction, selectedTransactions], | ||
|
@@ -185,13 +207,21 @@ export function TransactionList({ | |
); | ||
} | ||
|
||
function SelectedTransactionsFloatingActionBar({ transactions, style }) { | ||
type SelectedTransactionsFloatingActionBarProps = { | ||
transactions: readonly TransactionEntity[]; | ||
style?: CSSProperties; | ||
}; | ||
|
||
function SelectedTransactionsFloatingActionBar({ | ||
transactions, | ||
style = {}, | ||
}: SelectedTransactionsFloatingActionBarProps) { | ||
const editMenuTriggerRef = useRef(null); | ||
const [isEditMenuOpen, setIsEditMenuOpen] = useState(false); | ||
const moreOptionsMenuTriggerRef = useRef(null); | ||
const [isMoreOptionsMenuOpen, setIsMoreOptionsMenuOpen] = useState(false); | ||
const getMenuItemStyle = useCallback( | ||
item => ({ | ||
<T extends string>(item: MenuItemObject<T>) => ({ | ||
...styles.mobileMenuItem, | ||
color: theme.mobileHeaderText, | ||
...(item.name === 'delete' && { color: theme.errorTextMenu }), | ||
|
@@ -326,16 +356,18 @@ function SelectedTransactionsFloatingActionBar({ transactions, style }) { | |
let displayValue = value; | ||
switch (name) { | ||
case 'account': | ||
displayValue = accountsById[value]?.name ?? value; | ||
displayValue = | ||
accountsById[String(value)]?.name ?? value; | ||
Comment on lines
+359
to
+360
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add type safety checks for number conversion. The number conversion for - displayValue = integerToCurrency(Number(value));
+ const numValue = Number(value);
+ if (Number.isNaN(numValue)) {
+ throw new Error(`Invalid amount value: ${value}`);
+ }
+ displayValue = integerToCurrency(numValue); Also applies to: 363-364, 367-367, 370-370 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated: c06df65 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @leoltl Thank you for implementing the number validation! Your solution to gracefully handle invalid numbers by preserving the original value is even better than throwing an error. The changes look good. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script:
Length of output: 1269 If you found this review helpful, would you consider giving us a shout-out on X? |
||
break; | ||
case 'category': | ||
displayValue = categoriesById[value]?.name ?? value; | ||
displayValue = | ||
categoriesById[String(value)]?.name ?? value; | ||
break; | ||
case 'payee': | ||
displayValue = payeesById[value]?.name ?? value; | ||
displayValue = payeesById[String(value)]?.name ?? value; | ||
break; | ||
case 'amount': | ||
displayValue = integerToCurrency(value); | ||
displayValue = integerToCurrency(Number(value)); | ||
break; | ||
case 'notes': | ||
displayValue = `${mode} with ${value}`; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
category: Maintenance | ||
authors: [leoltl] | ||
--- | ||
|
||
Refactoring the mobile TransactionList component to typescript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would also work instead of exporting the type
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joel-jeremy Thanks for the second review and suggestion! Unfortunately the proposed change doesn't seem to work. 😢

I believe it is because the type of
'NameType'
in the first argument of the Menu's'getItemStyle'
is a generic that bounds to the actualMenu
instance (i.e. TS resolving the type from theitems
theMenu
instance was passed). Whereas it isunknown
type when it is not bounded to theMenu
instance.I am open to other suggestion, but so far setting this anonymous function with generic type is only way I get typing to work/ agree.